-
Notifications
You must be signed in to change notification settings - Fork 47
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
Dynesty warnings added #1324
Dynesty warnings added #1324
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1324 +/- ##
============================================
- Coverage 81.32% 53.47% -27.86%
============================================
Files 153 153
Lines 12515 12529 +14
============================================
- Hits 10178 6700 -3478
- Misses 2337 5829 +3492 ☔ View full report in Codecov by Sentry. |
Do we want to add this flag only in the dynesty sampler? I think it would be better to add it on the objective level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to merge with just the warning and leave the objective_type
things to a new PR where objective attributes are added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine then as a temporary fix for the dynesty sampler and to have some warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Good temporary solution for dynesty.
I have a general question, possibly stupid.
As much as I understand, the OBJECTIVE_NEGLOGLIKE and NEGLOGPOST constants are only passed to the dynesty sampler upon construction such that it knows whether it needs to remove the priors from the objective function when calculating the likelihood.
My question is whether the constants are necessary? Why not define the loglikelihood of dynesty to always remove the prior in cases a prior exists? The only case when it should not remove the prior from the objective is when it does not exist, right? I might be missing something.
If you load a petab problem, that should work fine. But if you define your objective yourself (which could be the nllh) and you define |
# Conflicts: # test/sample/test_sample.py
The wrapper for the dynesty sampler differs significantly to the other samplers in pyPESTO. Usually the samplers expect the objective to be the posterior and
x_priors
to be defined, from which they compute the likelihood if needed. I added the argumentobjective_type
to the sampler to switch between posterior and likelihood with default set to posterior. I also added checks in theinitialize
function to very ifx_priors
is defined in the problem or not.Furthermore, I added a warning that by default, a uniform prior with bounds specified in the problem is assumed. This warning was missing, but already mentioned in the
prior_transform
function.