Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix caching bug in signals.deterministic_signals.Deterministic #325

Merged
merged 6 commits into from
Nov 4, 2022

Conversation

vallis
Copy link
Collaborator

@vallis vallis commented Nov 3, 2022

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
Copy link

codecov bot commented Nov 3, 2022

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

@@           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 Coverage Δ
enterprise/signals/deterministic_signals.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4a651a...50a12b2. Read the comment docs.

@vallis vallis merged commit f1ca78e into nanograv:master Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants