-
Notifications
You must be signed in to change notification settings - Fork 122
Description
Hi all,
#864 attempted to fix issues with how joint priors are handled in ConditionalPriorDict and introduce new functionality which would let JointPriors themselves be defined conditional. The PR tried to do too much at once, and the solutions were unnecessarily bloated. However, I believe some of the fixes are important, and the added functionality would be worthwhile.
So, this is my attempt to break things done into more easily digestible chunks and smaller, simpler PR. Here, I'll only start with the bugs of the current implementation before moving on to new functionality. All the linked PR's are independent but should be compatible with each other.
Bugs/Issues in ConditionalPriorDict.rescale if any prior is dependent onJointPrior('s) :
- Currently, if any prior in
ConditionalPriorDictdepends on aJointPrior, the correct ordering of the keys is not ensured for rescaling because all keys associated with theJointPriorneed to be rescaled first. - Even if the order of keys was correct, rescaling would fail because the
least_recently_sampledproperty of the joint priors is not updated before it is accessed byget_required_variables.
I have addressed these issues in #1023
General bugs if not all names associated with a JointPrior are sampled/requested for probability calculation/requested for rescaling:
JointPriorandBaseJointPriorDistkeep track of the names that have been requested internally. Once all names associated with aBaseJointPriorDist-instance have been requested, this internal storage is reset. This can lead to unexpected behavior if only a subset of the names is requested. For instance, if one wants to sample a subset of the JointPrior-names repeatedly, the returned samples would never change (even if the requested size changes). The easiest solution would be to only considerConditionalPriorDicts resolved if for any includedJointPriorall names associated withJointPrior.distare also included.- The above solution does not work for
PriorDict(which would also not necessarily rescale to the correct order). Since any individualJointPrioris essentially a conditional random variable, I'd suggest to limit joint priors toConditionalPriorDict.
I have addressed only the ConditionalPriorDict case in #1024
Bugs/Issues for rescaling/sampling/evaluating subsets of keys:
- Currently, if a subset of the prior-keys is requested (eg. 3 keys for a ConditionalPriorDict with 4 priors) and a theta with the appropriate length (3) is passed, rescaling fails because the function attempts to rescale all non-fixed priors. Also, it would be necessary to check if the subset of requested keys can be resolved.
- The current implementation would treat any
ConditionalBetaDistributionas a "fixed" prior and add it to any prior evaluation, which could lead to wrong results.
I have addressed these issues in #1025 and have also streamlined the checking to see if subsets are resolved, as sampling/evaluations/rescaling should all do this same check.