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

Add prior_structure argument to optimize_prior_precision #123

Merged
merged 4 commits into from
Sep 1, 2023

Conversation

runame
Copy link
Collaborator

@runame runame commented Apr 14, 2023

Changes:

  • Add a helper function fix_prior_prec_structure for marglik optimization.
  • Add prior_structure argument to optimize_prior_precision, such that we can easily optimize a layer-wise or diagonal prior precision post-hoc.
  • Rename the method CV for optimize_prior_precision to gridsearch.

@runame runame added the enhancement New feature or request label Apr 14, 2023
@runame runame requested a review from aleximmer April 14, 2023 18:47
@aleximmer
Copy link
Owner

Should we set the grid search as default and/or put a warning for post hoc prior precision optimization since the method has not been thoroughly evaluated, yet, and can lead to issues apparently?

@runame
Copy link
Collaborator Author

runame commented Apr 17, 2023

Should we set the grid search as default and/or put a warning for post hoc prior precision optimization since the method has not been thoroughly evaluated, yet, and can lead to issues apparently?

Yeah, maybe setting gridsearch as the default makes sense. A warning is a bit annoying since we cannot easily detect if someone is using optimize_prior_precision for marglik optimization post-hoc or online.

@wiseodd
Copy link
Collaborator

wiseodd commented Apr 18, 2023

Rename the method CV' for optimize_prior_precision' to `gridsearch'.

A possible downside is that people have accustomed to CV. Since optimize_prior_precision is a public-facing method and is almost always called by users, I think it's a bit dangerous to rename it. At the very least we should support both.

@aleximmer aleximmer merged commit f6af736 into main Sep 1, 2023
@aleximmer aleximmer deleted the improve-marglik branch September 1, 2023 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants