PEP-8 renaming #1719
PEP-8 renaming #1719
Conversation
|
I rebased onto py3. |
|
I'm a bit confused re keeping backward compatibility of input files. Consider these two cases from an Arkane input file: transitionState(
label='TS3',
E0=(34.1, 'kcal/mol'),
...
)reaction(
label='CH2O+H=Methoxy',
reactants=['CH2O', 'H'],
products=['methoxy'],
transitionState='TS3',
)I think we all agree that the first case can be kept backward compatible, since Did I miss anything, or are there any insights for how to make these changes? |
|
That's correct, for RMG, I did not change any of the function signatures in Right now, I'm leaning towards keeping these input related functions as is, and not transitioning to new syntax with underscores. If we're keeping backward compatibility anyway, then this would not be a breaking change and could be introduced in a later minor release if we really wanted to. However, I think it might be more worthwhile to develop new yaml input and database files and have a single current -> yaml transition instead of going current -> underscores -> yaml. That way, users will only have to learn one new format. |
|
I don't remember if we brought it up or not: did we modify all scripts under both -Py and db repos, and ipython notebooks under RMG-Py? |
|
Thanks for bringing that up! I forgot to mention it. I think I updated RMG-Py scripts, but we should double check. RMG-database scripts have not been touched. Running the conversion script on those should (in theory) be sufficient. |
|
Agree that if a yaml change is coming for input files we should not have a second version of .py input files. Maintain current syntax, but accelerate the switch to yaml. |
alongd
force-pushed
the
py3_pep8
branch
12 times, most recently
from
81a0b4b
to
075182e
Compare
Codecov Report
@@ Coverage Diff @@
## py3 #1719 +/- ##
==========================================
+ Coverage 32.62% 32.65% +0.02%
==========================================
Files 87 87
Lines 26184 26158 -26
Branches 6875 6874 -1
==========================================
- Hits 8542 8541 -1
+ Misses 16677 16645 -32
- Partials 965 972 +7
Continue to review full report at Codecov.
|
|
@mliu49, I added the Arkane stuff to rmg2to3. If it looks OK feel free to squash (there's another fixup for Arkane PEP8) |
|
I fixed some more functions which were missed during the initial pass. I plan to squash commits together and merge on Monday. Feel free to make additional comments until then. |
To avoid shadowing the reserved keyword
Raising a NotImplementedError if called (only implemented for ReactionLibrary)
Using a syntax correction dictionary for on-the-fly conversions
Due to changes in gprof2dot API
This PR tries to standardize naming conventions across RMG to comply with PEP-8 recommendations. This addresses the issue of inconsistent naming in RMG and allows for more easily enforceable code style guidelines.
What has been done
Almost all names in RMG (global variables and functions, class attributes and methods, function and method arguments) have been changed to use lowercase_with_underscores. Class names are still CamelCase.
In most cases, the new name is a direct conversion, e.g.
isIsomorphic->is_isomorphic. In select cases, names were changed more substantially to improve clarity, e.g.getLabeledAtom->get_labeled_atomsandgetLabeledAtoms->get_all_labeled_atoms.I have also created a script (
scripts/rmg2to3.py) to facilitate the transition for codes dependent on RMG. It contains almost all names which have changed, except for some positional arguments. It uses regex to replace names, so there is some risk of replacing more than desired, but works for Cython and IPython notebooks as well.Scientific variables
One challenge was renaming scientific variables, including T, P, H, S, Cp, and E. Currently this isn't completely consistent, so some discussion may be needed. In this PR, T and P have generally been left as capital letters, including cases such as T_list or P_list. H, S, and Cp are generally lowercase, since they are usually not ambiguous. E has been challenging. Currently, E0 is still capital, while e_list is lowercase.
Input, database, and yaml files
Input and database parsing functions have not been changed yet. Backward compatibility is definitely something we want to keep. However, I think it is also worth discussing when (not if) we should transition those files to yaml as well. If we think that's something we want in the near future, it might not be worth changing input and database syntax for now.
Yaml files are slightly more complicated, since they are currently directly tied to classes and their attributes. We've had a bit of offline discussion, and we think that the way forward is to have a built in conversion function which will run if we fail to parse a yaml file using the new namespace the first time. It would also write an updated file for the user.
What needs to be done
This renaming process has been more challenging than I originally anticipated, so I would like help with the remaining tasks: