[BUG] Fix the handling of conditional priors dependent on JointPrior in ConditionalPriorDict #1023
Conversation
ColmTalbot
left a comment
ColmTalbot
left a comment
There was a problem hiding this comment.
Thanks for breaking this down, I think the logic makes sense. I just had a couple of minor questions to make sure we aren't doing unnecessary conversions between lists/sets.
It's great to see the end of safe_flatten!
| if isinstance(self[key], JointPrior): | ||
| # if joint prior, keep track if all names have been rescaled | ||
| distname = self[key].dist.distname | ||
| names = set(self[key].dist.names) |
There was a problem hiding this comment.
This feels like it shouldn't be needed, do we not impose that parameter names are unique?
There was a problem hiding this comment.
The cast to set is made because it does not matter in which order the keys occur in dist.names or joint[distname]. This could also be solved with a loop, if that would be preferred.
bilby/core/prior/dict.py
Outdated
| distname = self[key].dist.distname | ||
| names = set(self[key].dist.names) | ||
| if distname not in joint: | ||
| joint[distname] = [key] |
There was a problem hiding this comment.
Since this is only used as a set, why not just construct this directly as a set?
There was a problem hiding this comment.
True, and there were other redundant casts. Fixed with a new commit!
There was a problem hiding this comment.
Note: Noticed there was one more simplification possible
2 participants
This PR addresses the issues in
ConditionalPriorDict.rescaledescribed in the first item in #1026Just to reiterate, the addressed issues are:
ConditionalPriorDictdepends on aJointPrior, the correct ordering of the keys is not ensured for rescaling because all keys associated with theJointPriorneed to be rescaled first.least_recently_sampledproperty of the joint priors is not updated before it is accessed byget_required_variables.Note: The
safe_flattenfunction introduced in #979 toConditionalPriorDict.rescalehas a bug that if any of the values was an array, the returned flattened array takesresult[key].flatten()where it should usevalue.flatten(). However, I believe the flattening operation is redundant now anyway. It was previously only used to flatten the output ofJointPriors, I believe, which is now handled separately, and the flattening is never triggered by any of the test cases. (Note that if one attempts to rescale many samples at once, which works in many cases and is explicitly supported by some priors' doc-strings, this bug would lead to a wrong output. However, flattening would also not be necessary here.)