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 real time mbar disabling appending uncorrelated endstates #979

Merged
merged 8 commits into from
Apr 12, 2022

Conversation

ijpulidos
Copy link
Contributor

Description

In order to have the real time analysis YAML output with estimates for MBAR calculations and performance of the simulation, we had to disable uncorrelated end states creation.

Minor clean up: No minimization needed for HybridCompatibilityMixin.

Motivation and context

Resolves #916

How has this been tested?

tested locally. Waiting for benchmarks runs for real-case performance testing.

Change log

Output for real time mbar calculations and simulation performance in YAML format.

@ijpulidos ijpulidos requested a review from jchodera April 8, 2022 19:41
@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #979 (0d324c6) into main (54b0a3d) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@ijpulidos
Copy link
Contributor Author

Minimization is needed so far, but we will have to look into this in more detail in the future.

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.

Can you also link in the issue you created where we need to restore this once we've figured out how to generalize it?

Otherwise, looks good to me for now!

perses/app/setup_relative_calculation.py Show resolved Hide resolved
@@ -29,11 +27,11 @@ def __init__(self, *args, hybrid_factory=None, **kwargs):
self._hybrid_factory = hybrid_factory
super(HybridCompatibilityMixin, self).__init__(*args, **kwargs)

def setup(self, n_states, temperature, storage_file, minimisation_steps=100,
Copy link
Member

Choose a reason for hiding this comment

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

Did you say you had to keep minimisation_steps, or is this just unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, this was not being used, but I think it doesn't hurt to have it back, because it could actually break existing code if we don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. It wasn't being used but we should add it back since it could break currently existing code.

@ijpulidos
Copy link
Contributor Author

CC #978 for the creation of endstates issue. We have to consider re-enabling this once we refactor the create_endstates function.

@jchodera
Copy link
Member

Looks good, thanks!

@mikemhenry
Copy link
Contributor

Cool, once things pass we can merge this into main, then I'll update my cli branch. Once that is done I'll get that docker image built.

@ijpulidos ijpulidos enabled auto-merge (squash) April 12, 2022 13:34
@ijpulidos ijpulidos merged commit d3649d1 into main Apr 12, 2022
@ijpulidos ijpulidos deleted the fix-online-mbar-disabling-endstates branch April 12, 2022 13:38
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.

real-time analysis output
3 participants