devito icon indicating copy to clipboard operation
devito copied to clipboard

[WIP] api: fix Derivative dict substitution with self as key

Open mloubout opened this issue 1 year ago • 2 comments

Fixes #1995

mloubout avatar Sep 06 '22 15:09 mloubout

Codecov Report

Merging #1997 (b910367) into master (43ec3a8) will decrease coverage by 24.47%. The diff coverage is 7.14%.

@@             Coverage Diff             @@
##           master    #1997       +/-   ##
===========================================
- Coverage   87.86%   63.39%   -24.48%     
===========================================
  Files         214      214               
  Lines       36429    36443       +14     
  Branches     5497     5498        +1     
===========================================
- Hits        32009    23103     -8906     
- Misses       3904    12707     +8803     
- Partials      516      633      +117     
Impacted Files Coverage Δ
devito/finite_differences/derivative.py 67.79% <0.00%> (-21.68%) :arrow_down:
tests/test_derivatives.py 24.51% <12.50%> (-75.49%) :arrow_down:
tests/test_buffering.py 7.54% <0.00%> (-91.04%) :arrow_down:
tests/test_dimension.py 9.55% <0.00%> (-90.45%) :arrow_down:
tests/test_fission.py 11.76% <0.00%> (-88.24%) :arrow_down:
tests/test_caching.py 13.05% <0.00%> (-86.73%) :arrow_down:
tests/test_symbolics.py 14.46% <0.00%> (-85.54%) :arrow_down:
tests/test_checkpointing.py 14.59% <0.00%> (-85.41%) :arrow_down:
tests/test_skewing.py 13.88% <0.00%> (-85.19%) :arrow_down:
tests/test_differentiable.py 15.38% <0.00%> (-84.62%) :arrow_down:
... and 112 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Sep 06 '22 16:09 codecov[bot]

@EdCaunt ok so this is not as minor as anticipated. Mainly the cause of the issue you raised is that, unless you do subs(self, new) then the substitution is delayed and the dictionary stored as a property. it means if you do f.dx.subs({'a': 2}) then it will keep that dict and it will be part of the == comparison.

So in your case you end up with two different fdx. The original one and a new one that is "fdx but remember {fdx2: ....}" which even though in this case won't do anything leads to it being a different object. There isn't really an easy way out of it because some of the keys may not exist until after evaluation so there is no way to know if that dict needs to be saved to not.

So to summarize, your issue is just a side effect of the lazy evaluation and there is no straightforward way to get out of it.

PS: renaming as WIP and draft cuz may not be doable nicely.

mloubout avatar Sep 06 '22 18:09 mloubout