Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modify the regularization term and update par class #144

Closed
wants to merge 49 commits into from
Closed

modify the regularization term and update par class #144

wants to merge 49 commits into from

Conversation

JohnWangDataAnalyst
Copy link
Collaborator

@JohnWangDataAnalyst JohnWangDataAnalyst commented Jan 8, 2024

  1. fitting var_inv instead of var
  2. modified the regularization term becomes 1/var *(param - mean)^2
  3. change E-I from P to get channel EEG
  4. move some methods to Abstract for Parameters and NMM classes

@Andrew-Clappison
Copy link
Collaborator

Andrew-Clappison commented Jan 10, 2024

Some good things with simplifying the code. In particular, making progress on #107 and #108. This is also working on the task related to #115.

Is going to be a new PR focused on JR performance. In particular, improving performance with the custom_costs_JR objective function with priors. Using 1/var (or std) for the priors seemed to help.

For now, plan is to keep the options in the Par class to allow for having no priors, priors that are not fit, and priors that are fit. Might be re-discussed in the future, regarding possible alternative like setting priors with a small effect and different learning rate. May want to test various combinations first.

Might be good to discuss in more detail which things are to be simplified / what changes to the API.

May want to have cleaned up the PR into organized commits. Need to get all examples running before accepting the PR. Also, should keep the type check quality control in function arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants