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

Don't use online analysis to set offline analysis matrix dim #592

Closed
mikemhenry opened this issue Jun 7, 2022 · 7 comments · Fixed by #593
Closed

Don't use online analysis to set offline analysis matrix dim #592

mikemhenry opened this issue Jun 7, 2022 · 7 comments · Fixed by #593
Assignees
Milestone

Comments

@mikemhenry
Copy link
Contributor

mikemhenry commented Jun 7, 2022

We want to make sure the _offline_analysis works. _online_analysis is experimental and can be neglected for now. We need to make sure the matrix/array dimensions include the endstates for the offline analysis.

@mikemhenry mikemhenry added this to the 0.21.4 milestone Jun 7, 2022
@zhang-ivy
Copy link
Contributor

I think we had discussed that this can be part of the next milestone, actually.

@mikemhenry
Copy link
Contributor Author

mikemhenry commented Jun 7, 2022

Yes sorry! I made this issue as we were talking about it, I've bumped it!

@mikemhenry mikemhenry modified the milestones: 0.21.4, 0.22.0 Jun 7, 2022
@zhang-ivy
Copy link
Contributor

zhang-ivy commented Jun 7, 2022

We need to make sure the matrix/array dimensions include the endstates for the offline analysis.

Just going to elaborate on this.

The problem: Currently, we get the following error when online_analysis_interval != None and we use unsampled endstates: initial_f_k must be a 5-dimensional np array -- which is raised here on the pymbar side. The initial_f_k must be a 5-dimensional np array error comes from this line.

We basically want to avoid initializing initial_f_k (in offline analysis) with the self._last_mbar_f_k used for online analysis.

Going to assign this to @ijpulidos , but we can work together on this if needed.

@zhang-ivy
Copy link
Contributor

We may also want to add to the docstring an explanation of online vs offline analysis -- i.e. both compute the free energies, but online is a more a crude estimate.

@zhang-ivy
Copy link
Contributor

We'll also want to open an issue concerning adapting the online analysis to include unsampled endstates.

@zhang-ivy
Copy link
Contributor

Note that we may also want to consider opening an issue for renaming self.online_analysis_interval to self.offline_analysis_interval

@ijpulidos
Copy link
Contributor

Note that we may also want to consider opening an issue for renaming self.online_analysis_interval to self.offline_analysis_interval

Yes, I think this is already a TODO in the code, but I'll be opening an issue for this. The problem with this change is that it could break backwards compatibility (which we'd probably have to anyways, at some point).

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 a pull request may close this issue.

3 participants