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

Python3 #127

Merged
merged 14 commits into from Jul 31, 2018
Merged

Python3 #127

merged 14 commits into from Jul 31, 2018

Conversation

kain88-de
Copy link
Contributor

@kain88-de kain88-de commented Apr 26, 2018

except for two unit tests everything passes. Most changes should be harmless except for the tool registry, could be I broke that. But changing a dict during iteration isn't allowed in python 3 anymore.

fixes #44

@orbeckst
Copy link
Member

The Python 2.7 tests fail because of

  • six.long does not seem to be found
  • DataFrame does not have sort() (?!?)

The Python 3 tests have problems installing Gromacs. I am looking into it.

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.

@kain88-de can you think of a replacement for six.long() – it does not exist in the installed six.

The DataFrame.sort() error has been fixed in develop so this should be ok with a rebase. The Gromacs installation issues were raised in Becksteinlab/conda-gromacs-5.1.2#9 and Becksteinlab/conda-gromacs-5.1.2#10 (for Gromacs 5.1.2) and Becksteinlab/conda-gromacs-4.6.7#3 and Becksteinlab/conda-gromacs-4.6.7#4 (for 4.6.7).

gromacs/cbook.py Outdated
@@ -974,7 +975,7 @@ def edit_txt(filename, substitutions, newname=None):
else:
logger.debug("Deleting line %r", line)

target.seek(0L)
target.seek(six.long(0))
Copy link
Member

Choose a reason for hiding this comment

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

six.long() does not exist in six

Copy link
Member

Choose a reason for hiding this comment

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

In [4]: six.long
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-4-daab08199c2e> in <module>()
----> 1 six.long

AttributeError: 'module' object has no attribute 'long'

In [5]: six.__version__
Out[5]: '1.11.0'

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we actually need the longs... so I am removing them.

gromacs/cbook.py Outdated
@@ -1040,7 +1041,7 @@ def remove_molecules_from_topology(filename, **kwargs):
continue # remove by skipping
target.write(line)
if autogenerated and n_removed > 0:
target.seek(0L)
target.seek(six.long(0))
Copy link
Member

Choose a reason for hiding this comment

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

six.long() does not exist in six

gromacs/run.py Outdated
@@ -332,7 +333,7 @@ def check_mdrun_success(logfile):
except IOError:
return None
try:
log.seek(-1024L, 2)
log.seek(six.long(-1024), 2)
Copy link
Member

Choose a reason for hiding this comment

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

six.long() does not exist in six

@orbeckst orbeckst mentioned this pull request Apr 30, 2018
@kain88-de
Copy link
Contributor Author

If it works without the longs I would not use them anymore. Looks like the challenge now is python3 for your gromacs packages. I'm not sure what you mean by the DataFrame.sort issue.

@orbeckst
Copy link
Member

orbeckst commented May 1, 2018 via email

@kain88-de
Copy link
Contributor Author

Some tests used DataFrame.sort(), which does not exist anymore. But I fixed that.

oh yeah this was replaced by sort_values. I was looking for this last week myself.

about the gromacs packages. Are there some emails about this on the mailing list? We do have some good contacts conda-forge to help out with this.

@orbeckst
Copy link
Member

orbeckst commented May 1, 2018

about the gromacs packages. Are there some emails about this on the mailing list? We do have some good contacts conda-forge to help out with this.

No, that wasn't a public discussion. I can email you offline if you like.

@kain88-de
Copy link
Contributor Author

you can email me offline thanks.

@codecov-io
Copy link

codecov-io commented May 10, 2018

Codecov Report

Merging #127 into develop will increase coverage by 0.13%.
The diff coverage is 70.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #127      +/-   ##
===========================================
+ Coverage    48.14%   48.28%   +0.13%     
===========================================
  Files           24       24              
  Lines         4831     4836       +5     
  Branches       731      733       +2     
===========================================
+ Hits          2326     2335       +9     
+ Misses        2352     2341      -11     
- Partials       153      160       +7
Impacted Files Coverage Δ
gromacs/qsub.py 29.78% <0%> (ø) ⬆️
gromacs/fileformats/itp.py 30.86% <0%> (ø) ⬆️
gromacs/collections.py 27.27% <100%> (ø) ⬆️
gromacs/fileformats/xvg.py 29.52% <100%> (ø) ⬆️
gromacs/environment.py 70.31% <100%> (+0.47%) ⬆️
gromacs/run.py 67.93% <100%> (-0.95%) ⬇️
gromacs/core.py 57.93% <50%> (+3.19%) ⬆️
gromacs/config.py 65.18% <50%> (+0.02%) ⬆️
gromacs/cbook.py 22.03% <66.66%> (+0.22%) ⬆️
gromacs/utilities.py 53.2% <77.77%> (-0.18%) ⬇️
... and 5 more

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 ebe740e...8d6ad76. Read the comment docs.

@kain88-de
Copy link
Contributor Author

yay so python3 is running for at least one gromacs version

@kain88-de
Copy link
Contributor Author

Ok so with the new packages from bioconda the tests seem to run. There are still failures but there is progress.

@orbeckst
Copy link
Member

@ianmkenney has the same problem with Gromacs 2018 on conda in PR #130.

@kain88-de
Copy link
Contributor Author

most things are done now. Some string handling issues are left.

@kain88-de
Copy link
Contributor Author

@orbeckst this now sets all failing jobs to be allowed to fail. The old tests still work. Would you like to merge this as is? The last issues are with string handling but that might be better handled by someone who has used GromacsWrapper more then I did (I only needed the xvg reader once)

@orbeckst
Copy link
Member

I'd like to get the Python 3/Gromacs 4.6.5 tests to pass so that this PR really fixes #44.

I'll try to look at it over the next few days.

@kain88-de
Copy link
Contributor Author

I'm down to three errors now. The last ones fail when gromacs is rerun for amber forcefield. So something internally is messed up. Fixing these errors is not a lot more difficult then pocking in the debugger and some light googling.

@kain88-de
Copy link
Contributor Author

ok so yeah string handling fixes for python3 have broken python2 -.-

@kain88-de kain88-de force-pushed the python3 branch 2 times, most recently from d2d8693 to 9d20ea3 Compare May 18, 2018 09:06
@kain88-de
Copy link
Contributor Author

@ianmkenney do you know why the last three tests fail?

@orbeckst orbeckst mentioned this pull request Jun 12, 2018
@orbeckst
Copy link
Member

orbeckst commented Jun 12, 2018

I have no idea why these Test<FF>.test_mdrun tests fail under 3.6 but pass under 2.7. Why are the numerical results different? However, this looks as if it could be an easy win if it's something obviously stupid. ... and then this PR can be merged and we can start fixing 2018.

@kain88-de
Copy link
Contributor Author

I would need some help to investigate those errors. It would be helpful to know the exact gromacs commands that are executed for this test in python 2.7 and 3.6. Without knowing the library it's not straightforward to understand the program flow that is happening.

@kain88-de
Copy link
Contributor Author

@ianmkenney did you have a chance to fixup the gromacs errors?

@ianmkenney
Copy link
Member

@kain88-de I haven't had much time to look at this issue, I can take a look in the next couple of weeks hopefully

@orbeckst
Copy link
Member

Given our sluggish pace of development on GW (and the general "wild west coding" attitude in this project ;-) ) I would go ahead and merge and immediately raise an issue for Python 3.x correctness. It would be nice to merge with everything passing (#127 (comment)) but at least it will allow us to move forward.

Any objections?

@kain88-de
Copy link
Contributor Author

kain88-de commented Jul 30, 2018 via email

@orbeckst
Copy link
Member

I totally understand.

kain88-de and others added 13 commits July 30, 2018 17:57
there are still some Grompp errors I don't know about
Code used to have 0L and -1024L but this is not supported in Py3
and it does not seem to be necessary in Py2 as shown in the docs
https://docs.python.org/2/library/stdtypes.html#file.seek
- use binary mode reading and writing when necessary
- replace map operator
- explicit list
- fix run
- fix pickling
- .... and more
@orbeckst
Copy link
Member

I am rewriting the history to clean up the commits. I will force-push to your branch and then merge.

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.

None yet

4 participants