DEV: make sure all priors return float when needed#848
DEV: make sure all priors return float when needed#848ColmTalbot wants to merge 18 commits intomainfrom
Conversation
# Conflicts: # bilby/core/utils/calculus.py
ColmTalbot
changed the title
|
I've double checked the Fermi-Dirac prior CDF and it gives the correct expected result. |
mj-will
left a comment
mj-will
left a comment
There was a problem hiding this comment.
LGTM, I just have a couple of questions before I approve.
| for names in joint.values(): | ||
| values = list() | ||
| for key in names: | ||
| values = np.concatenate([values, result[key]]) | ||
| for key, value in zip(names, values): | ||
| result[key] = value |
There was a problem hiding this comment.
Could you help me understand what this loop is doing? I'm finding the logic of this part a bit hard to follow, it might benefit from a comment.
There was a problem hiding this comment.
Looking at it, I really don't think I wrote this and suspect I picked it up from elsewhere, I can't find where immediately.
JointPrior.rescale either returns an empty list or a list with values for all of the parameters of the joint distribution, this loop is unpacking these by first converting something that might look like, {a: [], b: [], c: [1, 2, 3, 4], d: []} -> [1, 2, 3, 4] -> {a: 1, b: 2, c: 3, d: 4}. Is that ordering something we can assume is enforced? Probably since, dicts are ordered in py3, but I haven't checked recently.
There was a problem hiding this comment.
Okay, I think I follow. Is this tested somewhere?
Either way, I think it would be useful to add a comment explaining this.
test/core/prior/prior_test.py
Outdated
| def test_cdf_float_with_float_input(self): | ||
| for prior in self.priors: | ||
| if ( | ||
| bilby.core.prior.JointPrior in prior.__class__.__mro__ |
There was a problem hiding this comment.
Why is this test skipped for JointPriors with no finite upper bound?
There was a problem hiding this comment.
Great question, I think this should probably skip for any joint prior as I think the CDF isn't defined for those.
asb5468
left a comment
asb5468
left a comment
There was a problem hiding this comment.
Left a few questions in the code. Wondering what github will do when I press "submit review" with "request changes" chosen.
|
@ColmTalbot it looks like there are conflicts following #849 |
|
Closing as #979 supercedes this. |
4 participants
For a long time, some prior classes have cast inputs to numpy arrays this MR removes that behaviour and adds a test to make sure that sampling and evaluating the CDF do not cast to array.
This caused an error when evaluating the CDF many times and then combining the results.
I also streamlined some other behaviour in applying prior constraints that were originally flagged by @cjhaster.
I noticed that the Fermi-Dirac CDF hadn't been implemented for some reason so I added that and added to the CI test suite.
Finally, there was an issue with sampling from joint distributions where the results could be returned out of order.
Copied from https://git.ligo.org/lscsoft/bilby/-/merge_requests/1285