Py3 only fixes breaking Py2 compatibility #1713

mliu49 · 2019-08-29T01:10:35Z

This PR contains changes necessary for full compatibility with Python 3, some of which require breaking Python 2 compatibility.

#1683, which replaces dde with chemprop, has been included in this PR. The purpose is to have a single PR which switches (the py3 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 the py3 branch.

mliu49 · 2019-08-29T01:16:52Z

Unresolved Issue 1: XYZ string generation in Arkane

======================================================================
FAIL: Test properly loading the ArkaneSpecies object and respective sub-objects
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/mjliu/Code/RMG-Py/arkane/commonTest.py", line 369, in test_create_and_load_yaml
    self.assertTrue('C 7.54e-14 1.193e-13 5.52e-14' in arkane_spc.xyz)
AssertionError: False is not true

----------------------------------------------------------------------

I commented out the failing assertion temporarily so that unit tests will pass and RMG-tests will run. The cause has been identified as a change in how floats are handled by str.format.

Unresolved Issue 2: Sorting Entry objects

I just discovered this while running a test job. I think we might need to do a global search for sorting and verify that all instances of implicit sorting of custom classes are addressed.

Traceback (most recent call last):
  File "/home/mjliu/Code/RMG-Py/rmg.py", line 185, in <module>
    main()
  File "/home/mjliu/Code/RMG-Py/rmg.py", line 179, in main
    rmg.execute(**kwargs)
  File "/home/mjliu/Code/RMG-Py/rmgpy/rmg/main.py", line 894, in execute
    trimolecularReact=self.trimolecularReact)
  File "/home/mjliu/Code/RMG-Py/rmgpy/rmg/model.py", line 646, in enlarge
    self.updateUnimolecularReactionNetworks()
  File "/home/mjliu/Code/RMG-Py/rmgpy/rmg/model.py", line 1756, in updateUnimolecularReactionNetworks
    network.update(self, self.pressureDependence)
  File "/home/mjliu/Code/RMG-Py/rmgpy/rmg/pdep.py", line 776, in update
    spec.generateStatMech()
  File "rmgpy/species.py", line 779, in rmgpy.species.Species.generateStatMech
  File "/home/mjliu/Code/RMG-Py/rmgpy/data/statmech.py", line 667, in getStatmechData
    statmech_model = self.getStatmechDataFromGroups(molecule, thermoModel)
  File "/home/mjliu/Code/RMG-Py/rmgpy/data/statmech.py", line 701, in getStatmechDataFromGroups
    return self.groups['groups'].getStatmechData(molecule, thermoModel)
  File "/home/mjliu/Code/RMG-Py/rmgpy/data/statmech.py", line 404, in getStatmechData
    for entry in group_count if group_count[entry] > 0])
TypeError: '<' not supported between instances of 'Entry' and 'Entry'

Note: This particular comparison issue was fixed, but there could still be others.

codecov · 2019-08-29T01:51:14Z

Codecov Report

Merging #1713 into py3 will decrease coverage by 8.79%.
The diff coverage is 44.66%.

@@            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

Impacted Files	Coverage Δ
rmgpy/pdep/network.py	`12.62% <0%> (ø)`	⬆️
rmgpy/molecule/symmetry.py	`0% <0%> (ø)`	⬆️
rmgpy/data/statmech.py	`42.39% <0%> (+0.67%)`	⬆️
rmgpy/rmg/pdep.py	`12.21% <0%> (ø)`	⬆️
rmgpy/yml.py	`15.71% <0%> (-0.12%)`	⬇️
rmgpy/molecule/group.py	`0% <0%> (ø)`	⬆️
rmgpy/tools/regression.py	`29.76% <0%> (ø)`	⬆️
rmgpy/tools/fluxdiagram.py	`9.91% <0%> (ø)`	⬆️
rmgpy/data/solvation.py	`65.2% <0%> (+1.53%)`	⬆️
rmgpy/data/kinetics/rules.py	`51.31% <0%> (+0.13%)`	⬆️
... and 37 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 af9dc41...6cfa689. Read the comment docs.

alongd · 2019-08-29T03:12:56Z

Why is the coverage significantly lower?

mliu49 · 2019-08-29T14:37:15Z

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.

alongd

Just a few comments.
Do we also need/want to convert all u""" instances in the database repo?

rmgpy/data/test_data/thermo/groups/group.py

rmgpy/data/thermo.py

rmgpy/ml/estimator.py

rmgpy/molecule/translatorTest.py

rmgpy/test_data/testing_database/solvation/libraries/solute.py

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.

mliu49 · 2019-08-29T17:11:49Z

Rebased onto py3. The two unresolved issues mentioned earlier have been fixed.

amarkpayne

I have taken a look through all of the commits and I have a few quick comments. Let me know what you think.

rmgpy/ml/estimator.py

amarkpayne · 2019-09-03T15:39:19Z

rmgpy/data/test_data/thermo/groups/adsorptionPt.py

@@ -2,8 +2,8 @@
 # encoding: utf-8

 name = "Surface Adsorption Corrections"
-shortDesc = u""


Can you also make this change in the documentation as well? For example, line 242 of database/introduction.rst.txt and many others

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

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)

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.

.travis.yml

setup.py

rmgpy/speciesTest.py

arkane/inputTest.py

rmgpy/rmg/model.py

Makefile

rmgpy/data/statmech.py

setup.py

mliu49 · 2019-09-05T20:22:51Z

I got this warning while running a test job on this branch:

/home/mjliu/Code/RMG-Py/rmgpy/tools/plot.py:211: RuntimeWarning: More than 20 figures have been opened. Figures created through the pyplot interface (`matplotlib.pyplot.figure`) are retained until explicitly closed and may consume too much memory. (To control this warning, see the rcParam `figure.max_open_warning`).
  fig = plt.figure()

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 matplotlib.pyplot.close.

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

mliu49 · 2019-09-10T22:08:36Z

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 environment.yml when available and environment_[os].yml otherwise. This way, all tests will pass. We can then remove the dde models from database at a later date.

@cgrambow, any thoughts about database?

cgrambow · 2019-09-11T14:30:12Z

I'd say just kick out the DDE models but no strong feelings either way.

amarkpayne · 2019-09-11T14:31:48Z

I made a quick comment about the file encoding lines, but otherwise once the TEMP commit issues get resolved I approve of merging this PR into py3 as long as @alongd and @cgrambow feel the same way.

I'll work with @mliu49 to review and merge the needed changes for RMG-tests

mliu49 · 2019-09-11T14:46:18Z

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.

mliu49 added Py3 Breaks Py2 labels Aug 29, 2019

mliu49 requested review from mjohnson541, alongd, amarkpayne and xiaoruiDong August 29, 2019 01:10

alongd mentioned this pull request Aug 29, 2019

Modified the ArkaneSpecies.update_xyz_string() method #1714

Merged

alongd reviewed Aug 29, 2019

View reviewed changes

Colin Grambow and others added 10 commits August 29, 2019 13:09

Switch ML estimator to chemprop

72b92a0

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.

Update documentation and ML example

3b7cb4c

Update meta.yaml

1522599

Revert Fix Travis builds by adding in missing miniconda dependencies

d0e5ca3

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

Update Python 3 environment

0b48cb7

Adjust directory structure for N4H6 in arkane examples

86f4bf9

Move PyDQED check to check_dependencies function

2c158ad

Refactor check_dependencies to be more modular

8d4d526

Remove u tag for unicode string from testing databases

f59814c

mliu49 force-pushed the py3_only branch from 05e87b1 to 704ccf1 Compare August 29, 2019 17:10

Change travis to only run py3 build, since py2 will always fail now

4e78f1d

mliu49 force-pushed the py3_only branch from b28c9fe to 1397600 Compare August 29, 2019 20:20

amarkpayne reviewed Sep 3, 2019

View reviewed changes

mliu49 commented Sep 4, 2019

View reviewed changes

setup.py Outdated Show resolved Hide resolved

amarkpayne force-pushed the py3_only branch from 6ccdf72 to 8bed808 Compare September 5, 2019 15:24

mliu49 added 13 commits September 10, 2019 17:58

Miscellaneous fixes to unit tests

90c6803

Improper syntax issues which didn't break in Python 2

Fix sorting when using machine learning thermo

f487687

Quantity objects cannot be implicitly sorted anymore

Fix type comparison

4589119

Fix sorting in generateReactionKey

df19793

None can no longer be sorted along with strings

Fix file opening using file

403b44c

The `file` function not longer exists

Fix interesting symmetry bug

aba04e0

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

Remove work_in_progress decorator from a test

cca187f

It is now passing for an unclear reason

Capture standard output when running functional tests

bcac42d

To clean up Travis output a bit Now, stdout will only be displayed if a test fails

Fix sorting of Quantity objects in main.execute

2dd0a79

Decode dot output when generating flux diagram

34b3bc0

Refactor removeVanDerWaalsBonds to not modify dict while iterating

6c8004e

Switch to Py3 compatible version of Q2DTor

bfb51b1

mliu49 force-pushed the py3_only branch from f23093b to f20d0d8 Compare September 10, 2019 21:59

mliu49 and others added 6 commits September 10, 2019 18:00

Remove uses of six.string_types

aa6243f

They were temporary for py2/3 compatibility They can be removed since we will only support py3 going forward

Make Entry.shortDesc and Entry.longDesc standard attributes

4baba99

Since we don't need str/unicode conversion anymore, there is no need to have it as a property.

Fix min usage with Entry objects

aaaa9b1

The Entry class does not have comparison methods implemented

Make the python 3 environment file the only environment file

7b4f66e

Remove xrange calls in chemkin.pyx

3d9fbdb

Close all matplotlib.pyplot figures

6cfa689

In Arkane's create_hindered_rotor_figure() method Also in rmgpy.stats and rmgpy.tools.plot Otherwise they accumulate in memory

mliu49 force-pushed the py3_only branch from f20d0d8 to 7d742cd Compare September 10, 2019 22:01

mliu49 force-pushed the py3_only branch from 7d742cd to 6cfa689 Compare September 11, 2019 17:06

amarkpayne approved these changes Sep 11, 2019

View reviewed changes

amarkpayne merged commit 28931a0 into py3 Sep 11, 2019

amarkpayne deleted the py3_only branch September 11, 2019 19:49

mliu49 mentioned this pull request Sep 23, 2019

Chemprop #1683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Py3 only fixes breaking Py2 compatibility #1713

Py3 only fixes breaking Py2 compatibility #1713

mliu49 commented Aug 29, 2019

mliu49 commented Aug 29, 2019 •

edited

Loading

codecov bot commented Aug 29, 2019 •

edited

Loading

alongd commented Aug 29, 2019

mliu49 commented Aug 29, 2019

alongd left a comment

mliu49 commented Aug 29, 2019

amarkpayne left a comment

amarkpayne Sep 3, 2019

amarkpayne Sep 3, 2019

amarkpayne Sep 3, 2019

mliu49 Sep 3, 2019

mliu49 commented Sep 5, 2019

mliu49 commented Sep 10, 2019

cgrambow commented Sep 11, 2019

amarkpayne commented Sep 11, 2019

mliu49 commented Sep 11, 2019

Py3 only fixes breaking Py2 compatibility #1713

Py3 only fixes breaking Py2 compatibility #1713

Conversation

mliu49 commented Aug 29, 2019

mliu49 commented Aug 29, 2019 • edited Loading

Unresolved Issue 1: XYZ string generation in Arkane

Unresolved Issue 2: Sorting Entry objects

codecov bot commented Aug 29, 2019 • edited Loading

Codecov Report

alongd commented Aug 29, 2019

mliu49 commented Aug 29, 2019

alongd left a comment

Choose a reason for hiding this comment

mliu49 commented Aug 29, 2019

amarkpayne left a comment

Choose a reason for hiding this comment

amarkpayne Sep 3, 2019

Choose a reason for hiding this comment

amarkpayne Sep 3, 2019

Choose a reason for hiding this comment

amarkpayne Sep 3, 2019

Choose a reason for hiding this comment

mliu49 Sep 3, 2019

Choose a reason for hiding this comment

mliu49 commented Sep 5, 2019

mliu49 commented Sep 10, 2019

cgrambow commented Sep 11, 2019

amarkpayne commented Sep 11, 2019

mliu49 commented Sep 11, 2019

mliu49 commented Aug 29, 2019 •

edited

Loading

codecov bot commented Aug 29, 2019 •

edited

Loading