Skip to content

[BUG] Correctly resolving ConditionalPriorDicts fails in several edge-cases #1026

@JasperMartins

Description

@JasperMartins

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 ConditionalPriorDict depends on a JointPrior, the correct ordering of the keys is not ensured for rescaling because all keys associated with the JointPrior need to be rescaled first.
  • Even if the order of keys was correct, rescaling would fail because the least_recently_sampled property of the joint priors is not updated before it is accessed by get_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:

  • JointPrior and BaseJointPriorDist keep track of the names that have been requested internally. Once all names associated with a BaseJointPriorDist-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 consider ConditionalPriorDicts resolved if for any included JointPrior all names associated with JointPrior.dist are also included.
  • The above solution does not work for PriorDict (which would also not necessarily rescale to the correct order). Since any individual JointPrior is essentially a conditional random variable, I'd suggest to limit joint priors to ConditionalPriorDict.

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 ConditionalBetaDistribution as 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions