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

Remove python 2 Support in MDPOW #178

Merged
merged 22 commits into from
Aug 17, 2021

Conversation

ALescoulie
Copy link
Contributor

@ALescoulie ALescoulie commented Aug 3, 2021

Removing Python 2 Compatibility

Since we now have a release supporting 2.7 and 3.x we can safely remove support for python 2 for future version. To begin that process I removed some of the ugly patching in the tests, and switched everything to future imports.

Still to do

  • Remove six dependency from code

@codecov
Copy link

codecov bot commented Aug 3, 2021

Codecov Report

Merging #178 (2d50f75) into develop (9c5b966) will decrease coverage by 0.19%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #178      +/-   ##
===========================================
- Coverage    73.14%   72.94%   -0.20%     
===========================================
  Files            9        9              
  Lines         1400     1386      -14     
  Branches       189      188       -1     
===========================================
- Hits          1024     1011      -13     
+ Misses         295      294       -1     
  Partials        81       81              
Impacted Files Coverage Δ
mdpow/__init__.py 100.00% <ø> (ø)
mdpow/config.py 71.75% <0.00%> (-0.43%) ⬇️
mdpow/filelock.py 89.74% <ø> (-0.26%) ⬇️
mdpow/forcefields.py 85.36% <0.00%> (-0.18%) ⬇️
mdpow/log.py 88.88% <ø> (-0.59%) ⬇️
mdpow/equil.py 80.45% <100.00%> (-0.08%) ⬇️
mdpow/fep.py 68.51% <100.00%> (-0.05%) ⬇️
mdpow/restart.py 73.10% <100.00%> (-0.45%) ⬇️
mdpow/run.py 62.14% <100.00%> (-0.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c5b966...2d50f75. Read the comment docs.

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.

Leave the from . import MODULE — that's the correct way to import a module inside a package. See other comments inline.

I'll try to get PR #182 and #176 merged (for 0.7.1) as soon as possible so that this can come afterwards and I don't have to do backports of fixes. You'll then also have an updated CHANGES to add your CHANGES in a 0.8.0 section.

mdpow/__init__.py Outdated Show resolved Hide resolved
mdpow/equil.py Outdated Show resolved Hide resolved
mdpow/equil.py Outdated Show resolved Hide resolved
mdpow/fep.py Outdated Show resolved Hide resolved
mdpow/restart.py Outdated Show resolved Hide resolved
mdpow/tests/test_analysis_alchemlyb.py Outdated Show resolved Hide resolved
# patch paths
elif sys.version_info.major == 2:
G = pickle.load(gsolv.open())
with open(gsolv, 'rb') as f:
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above

mdpow/tests/test_emin.py Outdated Show resolved Hide resolved
Comment on lines 3 to 1
from . import tempdir as td
import mdpow.tests.tempdir as td
Copy link
Member

Choose a reason for hiding this comment

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

. import

Comment on lines 3 to 1
from . import tempdir as td
import mdpow.tests.tempdir as td
Copy link
Member

Choose a reason for hiding this comment

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

. import

@orbeckst orbeckst added this to the 0.8.0 milestone Aug 4, 2021
@orbeckst orbeckst linked an issue Aug 4, 2021 that may be closed by this pull request
@orbeckst
Copy link
Member

orbeckst commented Aug 5, 2021

As soon as PR #186 is merged and 0.7.1 is released #181 , this PR is the next thing to complete so that everthing else can start with a leaner code base.

@ALescoulie
Copy link
Contributor Author

@orbeckst I replaced the imports as instructed, one issue is noticed is with exception syntax in the scripts, should I fix that here or make another pull request?

@orbeckst
Copy link
Member

orbeckst commented Aug 5, 2021

If you can make the scripts Py 3 then do that to, please.

I found it very useful to run them as described in the examples/README (in PR #186 ). We really need tests for the scripts ... #172

@ALescoulie
Copy link
Contributor Author

@orbeckst and @VOD555 How do these changes look. I still need to merge it with upstream develop.

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.

Somehow the merge/rebase reverted many changes that are in develop. Please check and rebase cleanly against develop.

mdpow/equil.py Outdated
Comment on lines 45 to 46
from . import config as config
from . import forcefields as forcefields
Copy link
Member

Choose a reason for hiding this comment

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

"as" not needed

Comment on lines 154 to 155
html_logo = "_static/mdpow-logo.png"
#html_logo = None
Copy link
Member

Choose a reason for hiding this comment

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

reverted commits on develop! undo

Comment on lines 159 to 160
html_favicon = "_static/mdpow.ico"
#html_favicon = None
Copy link
Member

Choose a reason for hiding this comment

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

reverted commits on develop! undo

Comment on lines 246 to 247
'https://gromacswrapper.readthedocs.io/en/latest': None,
'https://gromacswrapper.readthedocs.org/en/latest/': None,
Copy link
Member

Choose a reason for hiding this comment

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

reverted commits on develop (I think, check develop) undo?

Nevertheless, you should *always* check the topology and runinput
Nevertheless, you should _always_ check the topology and runinput
Copy link
Member

Choose a reason for hiding this comment

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

reverted commits on develop! undo

@@ -141,6 +141,7 @@ For current issues and open feature requests please look through the
fep
utilities
forcefields
tables
Copy link
Member

Choose a reason for hiding this comment

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

reverted commits on develop! undo

Comment on lines 131 to 134
compound and one solvent. All parameters except the solvent are specified in
the *RUNFILE*.

Arguments:

.. option:: RUNFILE

The runfile :file:`runinput.yml` with all configuration parameters.


Options:
Copy link
Member

Choose a reason for hiding this comment

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

everything: reverted commits on develop! undo

@orbeckst
Copy link
Member

@ALescoulie please ping me when you have addressed the comments and want me to review again.

@ALescoulie
Copy link
Contributor Author

ALescoulie commented Aug 13, 2021

@orbeckst I think I addressed the current issues. Sorry about the chaos that is scripts.txt, for some reason a bunch of spaces are being shown as deleted despite me just copying the original. For some reason one of the test_p_transfer tests is failing, but a few threads say it's an error with pandas 1.3.0.

edit: I forgot to update my own conda env, works on GitHub actions.

@orbeckst
Copy link
Member

I removed 2.7 status check requirements from develop.

Will finish review tonight (and then hopefully merge).

mdpow/fep.py Show resolved Hide resolved
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.

I have to apply the fixes where some commits reverted code on develop. Will check again and then merge.

mdpow/fep.py Outdated
Comment on lines 79 to 81
:members:
:inherited-members:

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:members:
:inherited-members:
:members:
:inherited-members:

mdpow/forcefields.py Outdated Show resolved Hide resolved
mdpow/forcefields.py Outdated Show resolved Hide resolved
Comment on lines 200 to 201
#: Solvents available in GROMACS; the keys of the dictionary
#: are the forcefields.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#: Solvents available in GROMACS; the keys of the dictionary
#: are the forcefields.
#: Solvents available in GROMACS; the keys of the dictionary
#: are the forcefields.

mdpow/forcefields.py Outdated Show resolved Hide resolved
mdpow/tests/test_analysis_alchemlyb.py Outdated Show resolved Hide resolved
mdpow/tests/test_emin.py Outdated Show resolved Hide resolved
scripts/mdpow-cfg2yaml.py Outdated Show resolved Hide resolved
scripts/mdpow-equilibrium Show resolved Hide resolved
scripts/mdpow-equilibrium Outdated Show resolved Hide resolved
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.

more reversals

mdpow/fep.py Outdated Show resolved Hide resolved
mdpow/fep.py Outdated Show resolved Hide resolved
mdpow/fep.py Outdated Show resolved Hide resolved
mdpow/fep.py Outdated Show resolved Hide resolved
mdpow/fep.py Outdated Show resolved Hide resolved
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.

more reverts

mdpow/fep.py Outdated Show resolved Hide resolved
mdpow/fep.py Outdated Show resolved Hide resolved
mdpow/forcefields.py Outdated Show resolved Hide resolved
@orbeckst orbeckst force-pushed the remove_python2 branch 2 times, most recently from 77a576f to 82590b9 Compare August 17, 2021 09:49
- removed Python 2 from setup.py metadata
- updated CHANGES
@orbeckst orbeckst merged commit fcff52a into Becksteinlab:develop Aug 17, 2021
ALescoulie added a commit to ALescoulie/MDPOW that referenced this pull request Sep 3, 2021
remove python 2

* fix Becksteinlab#177
* update setup.py and CHANGES

Co-authored-by: ALescoulie <[email protected]>
Co-authored-by: Oliver Beckstein <[email protected]>
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.

Remove Python 2 Support
2 participants