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

[WIP] Trying to make MBAR work better adaptively when it fails. #442

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mrshirts
Copy link
Collaborator

@mrshirts mrshirts commented Jun 20, 2022

  • Changed the way defaults are set up. Now the default is BFGS + adaptive.
  • Previously, it would run through solvers iteratively, assuming that multiple refinement is needed. However, if a solver is good, it should solve in one go. Therefore, we now loop through solvers until we find one that works.
  • So now, automatically tries BFGS adaptive and if the first one chosen fails within tolerance.
  • If multiple solvers are tried and fails, returns the best result.
  • Tries to address Should have \sum_n W_nk = 1 error with pymbar3.0.5 while analysis is working with pymbar3.0.3 #419 and MBAR estimator automatically choosing the usable method #424
  • Also makes the outputs and options of 'adaptive' more consistent with scipy methods to make the logic simpler.
  • Provides more error information when sum of weights is not normalized.

TODO:

  • scipy minimizer doesn't appear to be recognizing tolerance correctly - it will state success when the norm of the gradient is still relatively large (or at least larger than the tolerance). Different minimizers appear to use tolerance different ways? This needs to be analyzed.
  • Decide the order of the methods in default - should BFGS be tried first?
  • Maybe it's better to actually run multiple methods; start with something fast, then conclude with adaptive, which we know is more robust, once it's relatively close.
  • Make the output good enough to use/understand for now (we will move to logging system in 4.0).
  • See how different solvers behave and get tolerance to work correctly with them.

* Changed the way defaults are set up.
* Automatically tries both adaptive and L-BFGS-B if the first one chosen fails.
* If multiple solvers are tried, returns the best result.
* Makes adaptive more consistent with other methods.
@mrshirts mrshirts marked this pull request as draft June 20, 2022 01:08
@mrshirts mrshirts self-assigned this Jun 20, 2022
@orbeckst
Copy link

You're probably aware of @xiki-tempula 's implementation of "AutoMBAR" in alchemlyb. He did the testing and ultimately came up with the rationale for the order of methods that are tried.

@mrshirts
Copy link
Collaborator Author

Yes, I looked at that, but wasn't sure about logic. "hybr" is interesting since it is a root-finding method, which we haven't really looked at before.

Each method seems to handle tolerances a bit differently, and that's one thing that I've been trying to get to the bottom of. Calling adaptive at the end, which tends to continue a little further than other methods, seems to be a good way to makes sure that the results end up accurate, but I don't know if that's the "best" way to go.

@xiki-tempula
Copy link

xiki-tempula commented Jun 22, 2022

@mrshirts I called hybr first as it is be super fast and can solve most cases when the simulation is sufficiently converged. So calling it first to save time on the easy cases.
If hybr failed, I will call adaptive which could solve 99% of the cases.
Lastly, I will call BFGS. It is not as robust as adaptive but could solve the cases that adaptive cannot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants