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

Allow override t0 and g_t when computing free energy #585

Closed
wants to merge 1 commit into from

Conversation

zhang-ivy
Copy link
Contributor

@zhang-ivy zhang-ivy commented May 19, 2022

Description

The MultiStateAnalyzer automatically determines t0 and g_t here.

I have discovered that for my barnase:barstar repex simulations, t0 is always 1, which is too small. I'd like to be able to manually feed in t0 (and g_t) before retrieving the free energy.

This PR allows overriding t0 and g_t in the following way:

# Given a reporter, create an analyzer
analyzer = MultiStateSamplerAnalyzer(reporter, max_n_iterations=max_n_iterations)

# Override t0 and g_t
t0 = 1000
subsample_rate = 10
decorrelated_u_ln, decorrelated_N_l = analyzer._compute_mbar_decorrelated_energies(t0=t0, subsample_rate=subsample_rate)

# Compute free energy
f_ij, df_ij = analyzer.get_free_energy()

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

@zhang-ivy zhang-ivy requested review from ijpulidos and mikemhenry May 19, 2022 15:19
@zhang-ivy
Copy link
Contributor Author

@ijpulidos @mikemhenry : What do you think of my changes here? Is there a better way to allow the user to override these parameters?

@mikemhenry
Copy link
Contributor

Few ideas:

  1. Maybe we could improve the way these parameters are automatically determined? I have not looked into this at all, so that may be a complete non-stater.
  2. Letting a user specify these parameters seems reasonable, but I believe they belong in the analyzer constructor, or some public facing API. Maybe _compute_mbar_decorrelated_energies should be public? Is that a function that you call directly often or it the pattern mostly:
# Given a reporter, create an analyzer
analyzer = MultiStateSamplerAnalyzer(reporter, max_n_iterations=max_n_iterations)

# Compute free energy
f_ij, df_ij = analyzer.get_free_energy()

Another option would be to add some setters to analyzer so you could do something like:

# Given a reporter, create an analyzer
analyzer = MultiStateSamplerAnalyzer(reporter, max_n_iterations=max_n_iterations)

# Override t0 and g_t
t0 = 1000
subsample_rate = 10
analyzer.set_subsample_rate(subsample_rate)
analyzer.set_t0(t0)

# Compute free energy
f_ij, df_ij = analyzer.get_free_energy()

Really it all depends on if _compute_mbar_decorrelated_energies should be a public method or not.

@zhang-ivy
Copy link
Contributor Author

Re 1: I've already looked into ways to improve the way its automatically determined by replacing the potential energy timeseries that's fed in to _get_equilibration_data() with the state index trajectories and d_u/d_lambda. Neither of these seem to be helping. See thread: choderalab/perses#1000

Re 2: Yes, the normal calls are the ones you specified in the first code snippet. I just added the call to _compute_mbar_decorrelated_energies() because it required the least amount of changes to enable the feature I wanted. I think the second code snippet you sent could work, but not sure its necessary, we may need @jchodera 's feedback here.

@zhang-ivy
Copy link
Contributor Author

zhang-ivy commented May 23, 2022

Make n_equilibration_iterations and subsample_rate available in the MultiStateSamplerAnalyzer constructor
These should default to None.

If the user specifies all of them, don't call detect_equilibration.
If the user specifies the n_equilibration_iterations but not the subsample rate, still need to call detect_equilibration, using n_equilibration_iterations
If the user doesn't specify anything, call detect_equilibration.

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.

3 participants