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

About a more robust MBAR solver #170

Closed
xiki-tempula opened this issue Oct 12, 2021 · 5 comments · Fixed by #172
Closed

About a more robust MBAR solver #170

xiki-tempula opened this issue Oct 12, 2021 · 5 comments · Fixed by #172
Milestone

Comments

@xiki-tempula
Copy link
Collaborator

xiki-tempula commented Oct 12, 2021

Due to choderalab/pymbar#419, a number of downstream packages such as https://github.com/luancarvalhomartins/PyAutoFEP have to stick with alchemlyb 0.3.0 & pymbar 3.0.3. I'm thinking if it would be nice to have a more robust MBAR solver so that the downstream packages could use the latest version.

The pseudocode might be that

class MBAR(method=None):
    def fit(self,):
        if method is not None:
            pymbar.MBAR(method=method)
        else:
            try:
                pymbar.MBAR(method='adaptive')
            except:
                pymbar.MBAR(method='BFGS')

I'm aware that pymbar is working on solving this problem. I think it won't hurt to have an extra layer of protection at alchemlyb level.

@orbeckst
Copy link
Member

Where would we put it? Into our estimators?

Would we be using it ourselves?

@xiki-tempula
Copy link
Collaborator Author

xiki-tempula commented Oct 20, 2021

@orbeckst Yes, Into our estimators. When no method is provided, this fallback routine is used. When the method is supplied, it is used as is done now.

@mrshirts
Copy link

mrshirts commented Oct 21, 2021 via email

@xiki-tempula
Copy link
Collaborator Author

@mrshirts Thank you for the advice, I have adopted this comment in #162. However, for some cases, it still won't work for some cases, where switching to BFGS would solve the problem.

@orbeckst
Copy link
Member

@xiki-tempula I can see the merit of your approach, at least in the short-to-midterm (until anything changes in pymbar for good). Do you want to start a PR and then we can hash out details there?

We could always make it a separate estimator, such as "AutoMBAR", to distinguish it from the actual pymbar implementation, just so that there's less confusion when results change after an upgrade.

@orbeckst orbeckst mentioned this issue Oct 29, 2021
5 tasks
@orbeckst orbeckst added this to the 0.6.0 milestone Oct 29, 2021
orbeckst added a commit that referenced this issue Oct 29, 2021
* fix #170 
* new AutoMBAR estimator that tries different methods (hybr, adaptive, BGFS) until one succeeds
* refactored MBAR
* add logging for MBAR and AutoMBAR
* update docs
* add tests (using new dataset from PR alchemistry/alchemtest#60)
* update CHANGES

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants