pymc-devs / pymc

Bayesian Modeling and Probabilistic Programming in Python
https://docs.pymc.io/
Other
8.47k stars 1.97k forks source link

Do not consider dims without coords volatile if length has not changed #7381

Open JasonTam opened 1 week ago

JasonTam commented 1 week ago

Description

Allow coords defined by length only (no values) to be considered constant_coords if the new length matches.

Related Issue

Checklist

Type of change


📚 Documentation preview 📚: https://pymc--7381.org.readthedocs.build/en/7381/

JasonTam commented 1 week ago

The main part I feel uneasy about is having to call current_length.eval() since the length stored in model.dim_lengths is a pt shared variable

ricardoV94 commented 1 week ago

Also we need a test @JasonTam

JasonTam commented 4 days ago

@ricardoV94 I've pulled out the constant coord check logic into its own method in order to test in isolation. As I wasn't sure how to test this against the full sample_posterior_predictive method (or if that's even practical). Also, that method seems to have grown quite large.

codecov[bot] commented 4 days ago

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.19%. Comparing base (c2c46a7) to head (e7264e2). Report is 21 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/pymc-devs/pymc/pull/7381/graphs/tree.svg?width=650&height=150&src=pr&token=JFuXtOJ4Cb&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs)](https://app.codecov.io/gh/pymc-devs/pymc/pull/7381?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) ```diff @@ Coverage Diff @@ ## main #7381 +/- ## ========================================== - Coverage 92.36% 92.19% -0.17% ========================================== Files 102 103 +1 Lines 17190 17217 +27 ========================================== - Hits 15877 15873 -4 - Misses 1313 1344 +31 ``` | [Files](https://app.codecov.io/gh/pymc-devs/pymc/pull/7381?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) | Coverage Δ | | |---|---|---| | [pymc/sampling/forward.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7381?src=pr&el=tree&filepath=pymc%2Fsampling%2Fforward.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9zYW1wbGluZy9mb3J3YXJkLnB5) | `95.70% <92.30%> (-0.30%)` | :arrow_down: | ... and [15 files with indirect coverage changes](https://app.codecov.io/gh/pymc-devs/pymc/pull/7381/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs)
ricardoV94 commented 4 days ago

@JasonTam here is where we test this volatility logic: https://github.com/pymc-devs/pymc/blob/cbf1591295e030058a0a76108fef14ec5bc5e819/tests/sampling/test_forward.py#L113

Can you add something there instead. No need to do actual sampling, just make a scenario where a variable depends on a dim without coords (and then change it's length or not)

JasonTam commented 21 hours ago

@ricardoV94 I'm not sure how to write the test without actually sampling. In the similar tests under TestCompileForwardSampler, volatile vars are checked from the output of compile_forward_sampling_function. But that function already takes in constant_coords as a parameter.

The new logic of determining constant_coords can either be tested via the new low level function get_constant_coords (as currently written in the PR), or in the method which calls it, sample_posterior_predictive.

Maybe I'm misunderstanding what you mean.

ricardoV94 commented 20 hours ago

You're right @JasonTam, I was confused by the fact that the changes in constant data are checked inside the compile_forward_function but the coords are not. I guess this is the case because coords are not part of the graph while SharedVariables are.