-
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
Py3 only fixes breaking Py2 compatibility #1713
Conversation
|
Codecov Report
@@ Coverage Diff @@
## py3 #1713 +/- ##
=========================================
- Coverage 41.41% 32.62% -8.8%
=========================================
Files 167 87 -80
Lines 30118 26184 -3934
Branches 6421 6875 +454
=========================================
- Hits 12473 8542 -3931
+ Misses 16685 16677 -8
- Partials 960 965 +5
Continue to review full report at Codecov.
|
Why is the coverage significantly lower? |
Good question! It might be due to the fact that codecov is comparing between py2 results and py3 results, although it's not apparent what differences would affect coverage that much. |
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.
Just a few comments.
Do we also need/want to convert all u"""
instances in the database repo?
Only use two models, one that predicts enthalpy and one that predicts entropy and heat capacity simultaneously. Uncertainty cannot be estimated with the new models. If chemprop is not available, don't raise error until trying to use it.
Packages requiring libgcc have been rebuilt to require libgcc-ng instead. With this the conda solver can resolve the environment without needing pydot-ng and pygpu to be given explicitly
Primary goal is to not overwrite settings.pxi if it already exists. When using `cythonize`, Cython does a better job of figuring out if files have changed and need to be recompiled. Since we would rewrite settings.pxi every time upon calling `make solver`, Cython would think that it needs to recompile the entire solver module since it depends on that file. This change prevents excessive rewriting and recompilation.
Rebased onto |
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 have taken a look through all of the commits and I have a few quick comments. Let me know what you think.
@@ -2,8 +2,8 @@ | |||
# encoding: utf-8 | |||
|
|||
name = "Surface Adsorption Corrections" | |||
shortDesc = u"" |
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.
Can you also make this change in the documentation as well? For example, line 242 of database/introduction.rst.txt and many others
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.
Should we also remove this from RMG-database? It would make the database no longer compatible with python 2 going forward, but I think this is a good incentive for people to switch to the python 3 version of RMG-Py. I have not made my mind up on this, but rather just want to get the conversation going
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.
Additionally, remove unicode strings that use single quotes in the following files (there may be more). I searched u'
(leading space) to find these in PyCharm.
- molecule/draw.py
- reference.py
- documentation/source/conf.py (these ones may be okay)
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.
We should remove them from RMG-database, but it's not crucial. I removed them from the testing databases because it was straightforward to do.
Ideally, we would machine write the entire database, like we planned to do a while back. Another consideration is whether or not we want to change the database syntax.
I got this warning while running a test job on this branch:
Maybe this resulted from switching some plotting functionality from pylab to pyplot? Regardless, we should probably explicitly close the figure anytime we plot something, using |
Improper syntax issues which didn't break in Python 2
Quantity objects cannot be implicitly sorted anymore
None can no longer be sorted along with strings
The `file` function not longer exists
The for loop over the bond dictionary would go through too many iterations because we were modifying the dictionary while looping This worked in Python 2, possibly because dictionaries weren't ordered
It is now passing for an unclear reason
To clean up Travis output a bit Now, stdout will only be displayed if a test fails
The InChI class is defined as a subclass of str How pickling worked before is a mystery... Due to some change in objects and/or pickling in Python 3, this functionality no longer works as expected. Pickling of InChI/AugmentedInChI objects is not critical, so just mark as work_in_progress for now.
They were temporary for py2/3 compatibility They can be removed since we will only support py3 going forward
Since we don't need str/unicode conversion anymore, there is no need to have it as a property.
The Entry class does not have comparison methods implemented
In Arkane's create_hindered_rotor_figure() method Also in rmgpy.stats and rmgpy.tools.plot Otherwise they accumulate in memory
I went ahead and squashed the fixes and rebased. I would like to get this in asap so we can move on to finalizing other stuff. One last thing to address is how we want to handle the two temp commits. Since this branch incorporates chemprop and makes changes to environment files, both RMG-database and RMG-tests require updates. My proposal is to add the chemprop models into RMG-database without removing the dde models yet, so that python 2 and python 3 builds will work, and to modify RMG-tests to use @cgrambow, any thoughts about database? |
I'd say just kick out the DDE models but no strong feelings either way. |
If we remove the dde models, all master based RMG-Py builds, RMG-database builds, and RMG-tests builds will fail until we merge the py3 branch. If anyone were to pull current database during this time, their install would also be broken. If the time between merging this PR into py3 and merging py3 into master were to be less than 24 hours, I think it would be acceptable, but I feel like that is unlikely. |
This PR contains changes necessary for full compatibility with Python 3, some of which require breaking Python 2 compatibility.
#1683, which replaces
dde
withchemprop
, has been included in this PR. The purpose is to have a single PR which switches (thepy3
branch) from being fully functional in Python 2 to fully functional in Python3.As far as testing goes, I think the requirement for merging this into the
py3
branch will simply be reviewer approval and passing standard Travis tests. Once that's done, more rigorous testing will be done on thepy3
branch.