Fix find_constants excluding discrete observed data from constant_data - #8368
Open
genrichez wants to merge 1 commit into
Open
Fix find_constants excluding discrete observed data from constant_data#8368genrichez wants to merge 1 commit into
genrichez wants to merge 1 commit into
Conversation
For discrete distributions (Categorical, Poisson, Binomial), the observed value variable is wrapped in a Cast op. The old code checked 'var in value_vars' which only matched the Cast output, not the original pm.Data variable. This caused discrete observed data to leak into the constant_data group of InferenceData. Fix by walking ancestors of each observed RV's value variable and collecting any data_vars found there. This correctly identifies observed data regardless of whether a Cast op sits between the data var and the value var. Closes pymc-devs#7851
Comment on lines
+510
to
+512
| trace = pm.sample_prior_predictive(100, return_inferencedata=False) | ||
|
|
||
| inference_data = to_inference_data(prior=trace, model=model, log_likelihood=False) |
Contributor
There was a problem hiding this comment.
Why the return_inferencedata=False. Does this occur with True?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8368 +/- ##
==========================================
- Coverage 91.79% 88.16% -3.64%
==========================================
Files 128 128
Lines 21007 21010 +3
==========================================
- Hits 19283 18523 -760
- Misses 1724 2487 +763
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #7851
Problem
When using discrete distributions (Categorical, Poisson, Binomial) with
pm.Dataas observed, the data variable incorrectly ends up in theconstant_datagroup of InferenceData. It should only appear inobserved_data.Cause
For discrete distributions, the observed value variable gets wrapped in a
Castop (e.g.Cast{int64}(y_obs)). The old code infind_constantscheckedvar in value_vars, butvalue_varscontains the Cast output, not the originalpm.Datavariable. So the identity check fails and the data var slips through intoconstant_data.For continuous distributions this worked fine because the value variable IS the data var directly (no Cast in between).
Fix
Instead of checking if a data var is directly in
value_vars, walk the ancestors of each observed RV's value variable and collect anydata_varsfound there. This catches the data var regardless of whether a Cast (or any other transform) sits between it and the value var.Tests
test_discrete_observed_not_in_constant_dataas a regression test for the reported bugtest_observed_data_also_constantto reflect that observed data should not appear inconstant_dataeven when also used in the generative graph (per discussion in BUG:find_constantshelper function is inconsistent #7851)tests/backends/test_arviz.pypass