search

nanograv / enterprise

ENTERPRISE (Enhanced Numerical Toolbox Enabling a Robust PulsaR Inference SuitE) is a pulsar timing analysis code, aimed at noise analysis, gravitational-wave searches, and timing model analysis.
https://enterprise.readthedocs.io
MIT License
67 stars 67 forks source link

Fix caching bug in signals.deterministic_signals.Deterministic #325

Closed vallis closed 2 years ago

vallis commented 2 years ago

This bug will cause the wrong delays to be returned when signals.deterministic_signals.Deterministic.get_delay() is called repeatedly with the same parameters (e.g., when computing finite-difference partial derivatives). It wouldn't be a problem when every set of parameters is new, as in MCMC sampling.

Reproducible symptom: when issuing the sequence

xxret = deterministic_instance.get_delay(xx)
yyret = deterministic_instance.get_delay(yy)
xxret2 = deterministic_instance.get_delay(xx)

one finds that yyret != xxret (correct) but xxret2 = yyret (wrong!).

Cause: that signals.deterministic_signals.Deterministic computes the delay and stores it in an instance variable, then the caching mechanism (signal.cache_call) saves a reference to the instance variable. So the second call (get_delay(yy)) ends up modifying the cached value for xx.

Cure: The fix is for get_delay to return a new array whenever it's called (this won't prevent caching to function correctly). In general, any Signal that uses caching must return new numpy arrays if these are functions of the parameters. It's only OK to return references to instance variables for arrays that never change.

codecov[bot] commented 2 years ago

Codecov Report

Merging #325 (50a12b2) into master (b4a651a) will increase coverage by 0.00%. The diff coverage is 100.00%.

Additional details and impacted files [![Impacted file tree graph](https://codecov.io/gh/nanograv/enterprise/pull/325/graphs/tree.svg?width=650&height=150&src=pr&token=7Sjk8cLA85&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv)](https://codecov.io/gh/nanograv/enterprise/pull/325?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv) ```diff @@ Coverage Diff @@ ## master #325 +/- ## ======================================= Coverage 88.37% 88.37% ======================================= Files 13 13 Lines 3011 3012 +1 ======================================= + Hits 2661 2662 +1 Misses 350 350 ``` | [Impacted Files](https://codecov.io/gh/nanograv/enterprise/pull/325?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv) | Coverage Δ | | |---|---|---| | [enterprise/signals/deterministic\_signals.py](https://codecov.io/gh/nanograv/enterprise/pull/325/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv#diff-ZW50ZXJwcmlzZS9zaWduYWxzL2RldGVybWluaXN0aWNfc2lnbmFscy5weQ==) | `100.00% <100.00%> (ø)` | | ------ [Continue to review full report at Codecov](https://codecov.io/gh/nanograv/enterprise/pull/325?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/nanograv/enterprise/pull/325?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv). Last update [b4a651a...50a12b2](https://codecov.io/gh/nanograv/enterprise/pull/325?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv).