Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

pymbar4 #268

Merged
merged 21 commits into from
Dec 12, 2022
Merged

pymbar4 #268

merged 21 commits into from
Dec 12, 2022

Conversation

xiki-tempula
Copy link
Collaborator

@xiki-tempula xiki-tempula commented Nov 5, 2022

This PR allows the alchemlyb to use pymbar4. Fix #207
Should we make a dev branch for it or just merge to master?

@codecov
Copy link

codecov bot commented Nov 5, 2022

Codecov Report

Merging #268 (e466b34) into master (3bbbef2) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #268      +/-   ##
==========================================
+ Coverage   98.70%   98.74%   +0.03%     
==========================================
  Files          26       26              
  Lines        1777     1747      -30     
  Branches      382      380       -2     
==========================================
- Hits         1754     1725      -29     
+ Misses          3        2       -1     
  Partials       20       20              
Impacted Files Coverage Δ
src/alchemlyb/convergence/convergence.py 100.00% <100.00%> (ø)
src/alchemlyb/estimators/__init__.py 100.00% <100.00%> (ø)
src/alchemlyb/estimators/bar_.py 100.00% <100.00%> (ø)
src/alchemlyb/estimators/mbar_.py 100.00% <100.00%> (+1.63%) ⬆️
src/alchemlyb/preprocessing/subsampling.py 100.00% <100.00%> (ø)
src/alchemlyb/workflows/abfe.py 99.66% <100.00%> (-0.01%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@orbeckst
Copy link
Member

orbeckst commented Nov 7, 2022

My suggestion: For right now, make a parallel branch. If there are any fixes for 1.0 that we want/need to make then I'd rather keep these in the standard master branch. Once we have a 1.0.x out, we'll make a 1.x legacy branch for any potential support pymbar3-alchemlyb and switch master to pymbar4.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Very quick look — changes are suprisingly small!

Please

  • raise an issue to deprecate AutoMBAR in 1.1.0 and schedule for removal in 2.0.0.
  • update CHANGES (AutoMBAR and anything else that changes)
  • update documentation (index page)

When a specific method is set, AutoMBAR will behave in the same way
as MBAR.

.. versionadded:: 1.0.0
Copy link
Member

Choose a reason for hiding this comment

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

create issue & a PR with deprecation of AutoMBAR

@xiki-tempula xiki-tempula mentioned this pull request Dec 9, 2022
@@ -5,7 +5,7 @@ dependencies:
- python
- numpy
- pandas
- pymbar >=3.0.5,<4
- pymbar>4
Copy link
Member

@orbeckst orbeckst Dec 9, 2022

Choose a reason for hiding this comment

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

needs to be

Suggested change
- pymbar>4
- pymbar >=4

environment.yml Outdated
@@ -5,7 +5,7 @@ dependencies:
- python=3.8
- numpy
- pandas
- pymbar >=3.0.5,<4
- pymbar>4
Copy link
Member

Choose a reason for hiding this comment

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

=

setup.py Outdated
@@ -44,6 +44,6 @@
long_description_content_type='text/x-rst',
python_requires='>=3.8',
tests_require = ['pytest', 'alchemtest'],
install_requires=['numpy', 'pandas>=1.4', 'pymbar>=3.0.5,<4',
install_requires=['numpy', 'pandas>=1.4', 'pymbar>4',
Copy link
Member

Choose a reason for hiding this comment

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

=4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have to do >=4. Otherwise, I will be getting

Processing /home/docs/checkouts/readthedocs.org/user_builds/alchemlyb/checkouts/268
  Preparing metadata (setup.py): started
  Preparing metadata (setup.py): finished with status 'done'
Requirement already satisfied: numpy in /home/docs/checkouts/readthedocs.org/user_builds/alchemlyb/conda/268/lib/python3.11/site-packages (from alchemlyb==1.0.1+13.g52762bf.dirty) (1.23.5)
Requirement already satisfied: pandas>=1.4 in /home/docs/checkouts/readthedocs.org/user_builds/alchemlyb/conda/268/lib/python3.11/site-packages (from alchemlyb==1.0.1+13.g52762bf.dirty) (1.5.2)
ERROR: Ignored the following versions that require a different python version: 1.21.2 Requires-Python >=3.7,<3.11; 1.21.3 Requires-Python >=3.7,<3.11; 1.21.4 Requires-Python >=3.7,<3.11; 1.21.5 Requires-Python >=3.7,<3.11; 1.21.6 Requires-Python >=3.7,<3.11
ERROR: Could not find a version that satisfies the requirement pymbar==4 (from alchemlyb) (from versions: 2.0.0b0, 2.0.1b0, 2.1.0b0, 3.0.dev3, 3.0.2, 3.0.3, 3.0.4, 3.0.5, 3.1.0, 4.0.1)
ERROR: No matching distribution found for pymbar==4

.. versionadded:: 0.6.0
.. versionchanged:: 1.0.0
The ``estimator`` accepts uppercase input.
The default for using ``estimator='MBAR'`` was changed from
:class:`~alchemlyb.estimators.MBAR` to :class:`~alchemlyb.estimators.AutoMBAR`.
.. versionchanged:: 2.0.0
Use pymbar.MBAR instead of the AutoMBAR option.
Copy link
Member

Choose a reason for hiding this comment

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

unindent 1 space to line up

@@ -158,7 +141,7 @@ def overlap_matrix(self):
---------
pymbar.mbar.MBAR.computeOverlap
"""
return self._mbar.computeOverlap()["matrix"]
return self._mbar.compute_overlap()["matrix"]


class AutoMBAR(MBAR):
Copy link
Member

Choose a reason for hiding this comment

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

remove

@@ -1,4 +1,5 @@
import logging
from warnings import warn
Copy link
Member

Choose a reason for hiding this comment

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

not needed

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

completely remove AutoMBAR. With this PR, alchemlyb 2.0 will be started. We deprecated AutoMBAR with #284 so it will be gone in 2.0.

@xiki-tempula xiki-tempula marked this pull request as ready for review December 10, 2022 15:05
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Looking all fine. We're officially off to alchemlyb 2.0.0! 🚀

@orbeckst orbeckst added this to the 2.0.0 milestone Dec 12, 2022
@orbeckst orbeckst self-assigned this Dec 12, 2022
@orbeckst orbeckst merged commit 9b46988 into alchemistry:master Dec 12, 2022
@orbeckst
Copy link
Member

alchemlyb development is now officially 2.x

@xiki-tempula xiki-tempula deleted the feat__pymbar branch December 12, 2022 20:46
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upgrade to use pymbar 4
2 participants