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

More streamlined context caches usage #547

Merged
merged 6 commits into from Feb 14, 2022
Merged

Conversation

ijpulidos
Copy link
Contributor

@ijpulidos ijpulidos commented Feb 11, 2022

Description

Context caches for propagation and energy computations are expected to be handled only via MultiStateSampler instance attributes. Default behavior is to use global cache. If users want to avoid cycling they are expected to specify different context caches via the instance attributes.

Since we are not serializing context cache information, resuming simulations requires users to specify context cache attributes upon resuming.

Resolves #546

Todos

  • Implement feature / fix bug
  • Add tests
  • Update documentation as needed
  • Update changelogNotable points that this PR has either accomplished or will accomplish.

Status

  • Ready to go

@ijpulidos
Copy link
Contributor Author

Hmm, the test I designed for this is failing. It seems that using context_cache._lru._n_access is not the way to test this. Any ideas how to test this behavior? I'm just basically trying to test that propagating the replicas and the energy computation are using the context caches being specified.

@ijpulidos
Copy link
Contributor Author

I found out that ParallelTemperingSampler had its own _compute_replica_energies method that was hardcoded to use the global context cache. Fixed that and now all tests are passing locally. I'll wait for GHA workflow to give me the green flag, if so, this should be ready for revision.

Copy link
Member

@jchodera jchodera left a comment

Choose a reason for hiding this comment

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

Let's also make sure we document energy_context_cache and sampler_context_cache as class attributes of MultiStateSampler, note that they default to the global context cache, and potentially also add an example of how to override them with your own ContextCache in the docstring example.

@ijpulidos
Copy link
Contributor Author

@jchodera This should be ready for review, I updated the documentation as requested. Thanks!

Copy link
Member

@jchodera jchodera left a comment

Choose a reason for hiding this comment

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

Looks really good! Thanks!

@ijpulidos
Copy link
Contributor Author

I guess the change to main for the main branch messed up the codeclimate check. I think this has to be changed from the codeclimate side of things, @mikemhenry @jchodera can you check that please? I could also just merge it without this check using admin capabilities, in case we want to do that.

@jchodera
Copy link
Member

jchodera commented Feb 14, 2022 via email

@ijpulidos ijpulidos merged commit 7e6926f into main Feb 14, 2022
@ijpulidos ijpulidos mentioned this pull request Feb 14, 2022
@mikemhenry mikemhenry deleted the 546-streamlined-contextcache branch February 15, 2022 18:42
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.

More streamlined ContextCache usage
2 participants