Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Genextreme #84
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
Genextreme #84
Changes from 4 commits
f1a67b9
959677f
929bdfa
8cfc35d
85c3782
c43423f
13f9ade
b181815
14c8252
8471fca
e90be67
306ce90
2301015
0022a2f
544cf42
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This constraint depends on mu so it should probably be in a
switch(constraint, logp, -inf)
. Usually value-dependent constraints are treated elemwise while parameter based constraints are treated blockwise withcheck_parameters
(if one fails, everything fails), and we raise an error outside of sampling.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.
I've implemented this now, but not precisely sure the purpose:
check_parameters
is value-based, whileswitch(constraint...
is tensor-based, right? Anyway, please have a look.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.
The point is that the logp is usually defined for every possible
value
(although it can be zero for some combinations of value parameters, such as a halfnormal evaluated at-1
), so it should never "error" out. Logps can however be undefined for some parameter values, such as a halfnormal with negative sigma in which case we error out. We do that distinction in PyMC by using aswitch
for0
probability values andcheck_parameters
for parameter constraintsScipy is similar, but yields
nan
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 @ricardoV94 . I can see this alright. Nevertheless, the tests are failing only in the pymc test environment. Here are two cases:
Allowing xi to be in the R domain (mathematically true, but not in practice). The
npt.assert_almost_equal
is an absolute test of equality defaulting to 6 decimal places, and so we end up with silly outcomes like below, where the relative error is at the 61 - 47 = 15th order of magnitude down! This surely should be accepted?You can see the check from numpy here. It's worth noting that the numpy docs don't recommend using
assert_almost_equal
. For example,assert_allclose
would allow a relative tolerance to be specified, rather than an absolute one (in fact both or either).Restricting the domain of xi to the practical range of [-1,+1]. In this case running
check_logp
gives:where it is checking for an invalid value for xi (-2). But note that it passes when called directly:
and calling
check_parameters
passesI did have a look at adding a
-1 < xi < 1
check incheck-parameters
but I couldn't get it tow work with eitherxi > -1 & xi < 1
as a single check due to the tensors (is there an elementwise and? I couldn't find it in aesara docs), or...,x > -1, x<1,...
It would be great to have your input on this, since everything seems to be working fine until it enters the pymc test environment. Hopefully it is just a tweak to one of the conditions in the logp/logc functions perhaps?
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.
You can increase the test tolerange in
check_logp/ check_locdf
, with the kwargdecimal
. Do note that you may only be checking a subset of parameter combinations when testing locally (the default is a random subset of up to 100 combinations), so you can temporarily setn_samples=-1
to make sure all parameter combinations are testedThere 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.
@ricardoV94 good news: Using$\xi \in [-1,1]$ . All checks now pass!
at.and_
incheck_parameters
did the trick for the restricted (but practical) domain ofI can expand to the full domain of$\xi \in \mathbb{R}$ if the logp check is made relative instead of absolute as mentioned above.
In any case, thank you so much for your help on this. I think it's been 11 months in the pipeline, so hopefully we can get it merged?!
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.
I have thought about this sometime ago, but then faced issues. logps vary widely (and non-linearly) between 1e-700xs and 1e+x. More annoying, it's not uncommon to have logps around 0, in which cases relative comparisons or, on the other hand, more loose absolute comparisons become pretty meaningless.
If you have a more flexible solution that works in general that would be great. Certainly there are more principled ways of doing these accuracy checks.
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.
@ricardoV94
npt.assert_allcose
here allows for both a relative,rtol
and absoluteatol
tolerance to be set. Example:but this does not assert:
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.
Do want to try to open a PR on the main repo to change the logic then?
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.
Done: pymc-devs/pymc#6159