-
Notifications
You must be signed in to change notification settings - Fork 227
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
Upgrade Modules to Python 3 #1686
Conversation
Not complete yet, but I want to see what happens with the tests and Codacy checks |
Looks like a great start! Please can you post somewhere a list of steps taken to do similar on other unmerged pull requests, other branches, and other projects that build on or use RMG? |
Certainly! First off, I recommend taking a look at the google doc that @mliu49 sent out in the shared Slack channel, there are a lot of good details in there. The general workflow I am taking so far is as follows (@mliu49, @mjohnson541, @alongd, and @xiaoruiDong feel free to add anything or correct any mistakes I make):
One final note: the order here is important. In particular, do the style changes before the futurize changes so that the futurize changes are not masked in the git blame. |
Codecov Report
@@ Coverage Diff @@
## py3 #1686 +/- ##
==========================================
- Coverage 41.82% 41.75% -0.07%
==========================================
Files 167 167
Lines 29684 29725 +41
Branches 6203 6222 +19
==========================================
- Hits 12415 12413 -2
- Misses 16348 16385 +37
- Partials 921 927 +6
Continue to review full report at Codecov.
|
It should be noted that you shouldn't rename local variables because if you do so it will make rebasing on RMG an absolute nightmare. |
9720913
to
cbc2329
Compare
60d0fe9
to
b42bd3b
Compare
I have altered the author dates on these commits so that they show up in the easiest order to review them in. Note that in the "Futurize" commit I introduce the absolute import feature, but this is always removed in the next commit "replace relative imports". |
70dd9fe
to
764147c
Compare
Please note that all of the steps (PEP-8 reformatting, renaming local variables, futurize, replace relative imports) have been done for all files list here and with my name on them in the google doc. If you don't see a commit about it, it just means that the step was unnecessary for that file. I have also separated the commits out by groups of files to make it easier to review. @alongd feel free to do what you want with the Arkane commits at the end of this PR. |
Thanks! The Arkane commits were cherry-picked and rebased on top of the Arkane Py3 PR |
@alongd, can you confirm that it's safe to remove the last 5 commits from this branch? |
I confirm. They were implemented appropriately in the Arkane branch, split by content and squashed into existing commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a couple of comments. Two things which you could consider doing in all of these files (if applicable) are
- Replacing star imports (my approach has been to delete it and wait for PyCharm to flag unknown names)
- Replacing
import numpy
withimport numpy as np
for consistency
rmgpy/rmgobjectTest.py
Outdated
@@ -273,12 +273,12 @@ def setUp(self): | |||
self.scalar_dict = {'class': 'ScalarQuantity', 'value': 500.0, 'units': 'K'} | |||
|
|||
# Abbreviate name | |||
PRO = PseudoRMGObject | |||
pro = PseudoRMGObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically aliasing a class, so should it be CamelCase? (Bringing up for discussion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch! I am the one who wrote this, so I guess past me got it right then.
Also, I just realized that this abbreviation is not necessary, as without it this code will still be within the character limit. I will get rid of it entirely then.
rmgpy/rmg/outputTest.py
Outdated
from .model import CoreEdgeReactionModel, ReactionModel | ||
from .output import * | ||
from rmgpy.rmg.model import CoreEdgeReactionModel, ReactionModel | ||
from rmgpy.rmg.output import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would also like to replace star imports if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks!
Thanks for all of the comments @mliu49! You were right about removing the * imports, we were getting imports in very strange ways. I have rebased this branch on the current py3 branch, let me know what you think. |
rmgpy/rmg/inputTest.py
Outdated
|
||
from rmgpy.rmg.main import RMG | ||
from rmgpy.rmg import input as inp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the difference between this and import rmgpy.rmg.input as inp
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After doing some searching and some playing around in python, there is no difference between these two statements, and it is purely up to us as to which one we prefer. It is worth noting that if it wasn't for the as inp
statements in each that there would be a slight difference, as import rmgpy.rmg.input
would put rmgpy.rmg into the namespace, whereas from rmgpy.rmg import input
would not. There is an even more interesting but subtle effect if we were trying to import a module level variable (see the second from the top response here), but this does not apply in this case.
Given this, which one do we prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know!
I have a slight preference for import rmgpy.rmg.input as inp
since it's cleaner and seems a bit more straightforward. I always figured that from imports were mainly if you wanted something from within a file. Up to you whether or not to change it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I added a couple minor comments.
Are you planning to squash commits together?
rmgpy/rmg/main.py
Outdated
def loadInput(self, path=None): | ||
""" | ||
Load an RMG job from the input file located at `inputFile`, or | ||
from the `inputFile` attribute if not given as a parameter. | ||
""" | ||
from input import readInputFile | ||
from .input import readInputFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace with absolute import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch! I forgot to check import statements nested in the code. I did some further searching using regex in PyCharm and I didn't find any further instances in the files covered by this PR, so I think we have it all now.
rmgpy/rmg/main.py
Outdated
def loadThermoInput(self, path=None): | ||
""" | ||
Load an Thermo Estimation job from a thermo input file located at `inputFile`, or | ||
from the `inputFile` attribute if not given as a parameter. | ||
""" | ||
from input import readThermoInputFile | ||
if path is None: path = self.inputFile | ||
from .input import readThermoInputFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
rmgpy/rmg/main.py
Outdated
def saveInput(self, path=None): | ||
""" | ||
Save an RMG job to the input file located at `path`, or | ||
from the `outputFile` attribute if not given as a parameter. | ||
""" | ||
from input import saveInputFile | ||
if path is None: path = self.outputFile | ||
from .input import saveInputFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
rmgpy/rmg/model.py
Outdated
self.newSurfaceRxnsLoss = self.newSurfaceRxnsLoss | lost_surface_rxns | ||
|
||
return not ( | ||
self.newSurfaceRxnsAdd != set() or self.newSurfaceRxnsLoss != set() or self.newSurfaceSpcsLoss != set() or self.newSurfaceSpcsAdd != set()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should either be one line or split after each or
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I agree this is much better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments, I have made the fixes (including an error I made in the last rebase that caused the tests to fail, but I have made sure that everything is back to what it should be).
Before I push the commits, I can squash them if you would prefer. I wasn't sure if we were doing that before each merge into py3 or if we were going to squash the py3 branch before merging into master
rmgpy/rmg/inputTest.py
Outdated
|
||
from rmgpy.rmg.main import RMG | ||
from rmgpy.rmg import input as inp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After doing some searching and some playing around in python, there is no difference between these two statements, and it is purely up to us as to which one we prefer. It is worth noting that if it wasn't for the as inp
statements in each that there would be a slight difference, as import rmgpy.rmg.input
would put rmgpy.rmg into the namespace, whereas from rmgpy.rmg import input
would not. There is an even more interesting but subtle effect if we were trying to import a module level variable (see the second from the top response here), but this does not apply in this case.
Given this, which one do we prefer?
rmgpy/rmg/main.py
Outdated
def loadInput(self, path=None): | ||
""" | ||
Load an RMG job from the input file located at `inputFile`, or | ||
from the `inputFile` attribute if not given as a parameter. | ||
""" | ||
from input import readInputFile | ||
from .input import readInputFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch! I forgot to check import statements nested in the code. I did some further searching using regex in PyCharm and I didn't find any further instances in the files covered by this PR, so I think we have it all now.
rmgpy/rmg/main.py
Outdated
def loadThermoInput(self, path=None): | ||
""" | ||
Load an Thermo Estimation job from a thermo input file located at `inputFile`, or | ||
from the `inputFile` attribute if not given as a parameter. | ||
""" | ||
from input import readThermoInputFile | ||
if path is None: path = self.inputFile | ||
from .input import readThermoInputFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
rmgpy/rmg/main.py
Outdated
def saveInput(self, path=None): | ||
""" | ||
Save an RMG job to the input file located at `path`, or | ||
from the `outputFile` attribute if not given as a parameter. | ||
""" | ||
from input import saveInputFile | ||
if path is None: path = self.outputFile | ||
from .input import saveInputFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
rmgpy/rmg/model.py
Outdated
self.newSurfaceRxnsLoss = self.newSurfaceRxnsLoss | lost_surface_rxns | ||
|
||
return not ( | ||
self.newSurfaceRxnsAdd != set() or self.newSurfaceRxnsLoss != set() or self.newSurfaceSpcsLoss != set() or self.newSurfaceSpcsAdd != set()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I agree this is much better
Final note: the commit |
I'm still a bit undecided on the best way to handle commits for this process. Perhaps we can discuss tomorrow. |
Only covers the following files: chemkinTest.py constants*.py constraints*.py display.py exceptions.py quantityTest.py rmgObjectTest.py
Before we were getting it from a dependency of a dependency
Previous wildcard imports masked this issue. Thanks to @mliu49 for catching this and making this fix.
I have squashed the commits and rebased on the current py3 branch |
Changes