pymc icon indicating copy to clipboard operation
pymc copied to clipboard

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

Open JasonTam opened this issue 1 year ago • 7 comments

Description

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

Related Issue

  • [x] Closes #7376
  • [ ] Related to #

Checklist

Type of change

  • [ ] New feature / enhancement
  • [x] Bug fix
  • [ ] Documentation
  • [ ] Maintenance
  • [ ] Other (please specify):

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

JasonTam avatar Jun 21 '24 17:06 JasonTam

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

JasonTam avatar Jun 21 '24 17:06 JasonTam

Also we need a test @JasonTam

ricardoV94 avatar Jun 24 '24 08:06 ricardoV94

@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.

JasonTam avatar Jun 26 '24 22:06 JasonTam

Codecov Report

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

Project coverage is 92.18%. Comparing base (f719796) to head (b76087d). Report is 68 commits behind head on main.

Files with missing lines Patch % Lines
pymc/sampling/forward.py 93.33% 1 Missing :warning:
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7381      +/-   ##
==========================================
- Coverage   92.18%   92.18%   -0.01%     
==========================================
  Files         103      103              
  Lines       17249    17258       +9     
==========================================
+ Hits        15901    15909       +8     
- Misses       1348     1349       +1     
Files with missing lines Coverage Δ
pymc/sampling/forward.py 95.72% <93.33%> (-0.28%) :arrow_down:

codecov[bot] avatar Jun 26 '24 23:06 codecov[bot]

@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)

ricardoV94 avatar Jun 27 '24 09:06 ricardoV94

@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.

JasonTam avatar Jun 30 '24 19:06 JasonTam

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.

ricardoV94 avatar Jun 30 '24 20:06 ricardoV94

Thanks @JasonTam

ricardoV94 avatar Jul 07 '24 07:07 ricardoV94