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

Cache bug ylimit (fix: deepcopy instead of shallow copy) #678

Closed
wants to merge 3 commits into from

Conversation

dtnguyen2
Copy link
Contributor

Release Note

The function cache_model makes shallow copy of the cached function values, this PR added a couple of ```.copy()`` to make sure deep copies are made instead. This PR is a fix to issue #673

Note

In re-reading the code I also noticed that the setting of the _use_caching member of the class ArithmeticModel was not set consistently in the member function __set_state__

@dtnguyen2
Copy link
Contributor Author

Not sure why there are five file changes in this PR. The only expected changes should be in file: sherpa/models/model.py

@dtnguyen2
Copy link
Contributor Author

perhaps the following code

    def teardown(self):
        self._use_caching = False

in the file models/model.py should be changed to:

   def teardown(self):
       self._use_caching = True

also

@DougBurke
Copy link
Contributor

I would like the commit history to be cleared up before I review this.

I think either rebasing onto a clean, up to date master branch or cherry-picking the commit onto a clean, up to date, master branch should work.

@DougBurke
Copy link
Contributor

perhaps the following code

    def teardown(self):
        self._use_caching = False

in the file models/model.py should be changed to:

   def teardown(self):
       self._use_caching = True

also

I personally don't have a good feel for how the cache code and the setup/teardown methods are meant to interact. Previously I think the cache was only used for models with no free parameters, and then only within the context of a "setup/teardown" call - e.g. as done by a fit call. This is presumably why there is a unilateral "turn off cacheing" in the teardown method, in case it had been turned on by the setup method.

We've now changed things so that cacheing is expected to be on, so turning it off in teardown isn't very useful. However, I am not convinced that always turning it on is a great idea either. My current feeling would be that the teardown method shouldn't change the _use_caching attribute. In that case we may also want to remove the "turn on caching if there are no free parameters" logic in the setup method as well (since we expect the cache support to already be on), but I think this is getting to the point that we should be reviewing the cache code.

@wmclaugh wmclaugh added this to the 4.12.0 milestone Oct 18, 2019
Copy link
Contributor

@DougBurke DougBurke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the history be cleaned up before this gets merged?

@wmclaugh
Copy link
Contributor

@dtnguyen2 - Can you please rebase this so that it only touches the expected file (models.py) instead of 5 files?

Copy link
Contributor

@DougBurke DougBurke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code and test look fine. I thought you were going to clean up the history before I was to sign off?

@dtnguyen2
Copy link
Contributor Author

It was easier to clean up by issuing a new PR #705, so will close this PR

@dtnguyen2 dtnguyen2 closed this Nov 7, 2019
@wmclaugh wmclaugh removed this from the 4.12.0 milestone Jan 10, 2020
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.

None yet

3 participants