Add option to ignore suffix of gmx executables#165
Add option to ignore suffix of gmx executables#165orbeckst merged 15 commits intoBecksteinlab:masterfrom
Conversation
This adds two flags to pytest (one for making a link of gmx_mpi, the other for ignoring the suffix on the gmx executable); In order for this to work, the config file had to be edited before anything from gromacs was imported
|
It seems that I was overly ambitious trying to make what I thought were trivial changes to the ConfigParser and some uses of it. Seeing as you seem to want to continue Python 2 support, I can roll those changes back. |
|
@theavey what's the problem you're running into with Py2? I took a quick look and it looks like the |
|
So in Python 2, ConfigParser (or SafeConfigParser) are old-style classes (they don't inherit from It seems that one option is to add a second parent of |
Don't change behavior of the repo from which I forked This reverts commit e0c154a.
Codecov Report
@@ Coverage Diff @@
## master #165 +/- ##
==========================================
- Coverage 62.63% 62.52% -0.12%
==========================================
Files 21 21
Lines 4135 4136 +1
Branches 662 662
==========================================
- Hits 2590 2586 -4
- Misses 1359 1362 +3
- Partials 186 188 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #165 +/- ##
==========================================
+ Coverage 63.4% 63.54% +0.13%
==========================================
Files 21 21
Lines 4148 4161 +13
Branches 663 664 +1
==========================================
+ Hits 2630 2644 +14
+ Misses 1334 1333 -1
Partials 184 184
Continue to review full report at Codecov.
|
|
If the reason for moving the tests folder wasn't clear, I just wrote up a stackoverflow answer on a very related topic that might explain it more clearly. |
|
Hi @orbeckst, this PR is not critical, but if any changes need to be made, sooner would be better for me. This is just fresh in my mind now, and it would be easier to work on now instead of having to get back into this later. Thanks! |
| _cf_getbool = ConfigParser.getboolean | ||
| _UNSET = object() | ||
|
|
||
| def _getboolean(self, section, option, fallback=_UNSET, **kwargs): |
There was a problem hiding this comment.
this deserves a comment or two..
|
@theavey sorry, I hadn't seen the PR – just very busy at the moment, so wasn't checking my GitHub feed. Generally speaking, if you need a reaction include @orbeckst so that I get an email notification. @richardjgowers is already looking at the PR (THANK YOU Richard!!) and I won't really have time until the end of the week so whatever @richardjgowers decides is fine with me. |
|
And yes, I'd like to keep Python 2.7 compatibility – too much old code that uses it. |
|
@theavey sorry for the slow responses from me. Could you please resolve the conflicts that are flagged here? |
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
@theavey thanks for all the work.
Minor issues
- comment on the
_getboolean()method - I think the
scripts/*.pyfiles and thetemplates/*.shshould remain mode 755 – they are executable scripts. Or did you have a specific reason to change them to 644? - add an entry to CHANGES
- add yourself to AUTHORS
|
Please ping me or @richardjgowers when we should review again. Thanks! |
Add docstring and a few more comments on ConfigParser.getboolean for Python 2; Add entry to CHANGES; add entry to AUTHORS; fix accidental mode change to script files;
… into Becksteinlab-master
Fix conflicts because of updates to the base fork
|
Hi @orbeckst, The conflicts were just that I had moved the tests. I've added more comments on The mode changes were inadvertent. I think they must have just been 644 when I cloned the repository. I am not sure how else they would have been changed. I've added to CHANGES and AUTHORS. |
|
@theavey many thanks for your contribution! |
|
Of course! Thank you for the package |
|
P.S.: We wanted to write a short JOSS paper #147 – if you're interested in participating, add yourself to the issue and add to the PR #158. (Admittedly, I have been occupied with millions of other things and it's a bit slow going but it's going to happen eventually and anyone who contributed to the package has a chance to be on the paper.) |
3 participants
In response to my very old issue, this fixes #124.
This adds an option in the config file called append_suffix. If it is set to 'no', the suffix of the gmx executables (such as "_mpi" or "_d") will be ignored when making the tool names. It's not ideal to not differentiate them, but it can make code much more consistent across computers.
The largest change here is that in order to properly test this, the config file has to be changed before gromacs is imported. pytest makes all imports relative to the highest package it can find. Therefore, when
testswas nested in thegromacsfolder, the tests would be imported asgromacs.tests.test_.... This means that thegromacs.__init__.pyis run before even running any of the pytest setup functions. By moving the tests folder out of the gromacs package, the config file can be edited before thegromacs.__init__.pyis ever called.Because of this change, I've tried to refactor all the references to
gromacs.testsorgromacs/teststhat I could find.This PR another item to the travis testing matrix that tests this option. Using two new command line options to pytest, a link to the gmx executable called 'gmx_mpi' is put into the home folder and written in the tools section of the config file; and the config file is written to ignore the suffix of the gmx executables found.