Skip to content
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

Prettify #1744

Merged
merged 5 commits into from
Oct 10, 2019
Merged

Prettify #1744

merged 5 commits into from
Oct 10, 2019

Conversation

alongd
Copy link
Member

@alongd alongd commented Oct 3, 2019

Motivation or Problem

As noted in #1726 by @yunsiechung (Thanks!), Arkane's prettify functionality got broken in the Py3 transition.

Description of Changes

Instead of adapting Abstract Syntax Trees to the Py3 changes (which I don't know how to do right now), this PR suggests to use a simple combination of pprint.pformat and RMGObject.as_dict(). This might be advantageous since theoretically the object can be rebuilt using from_dict().

Testing

A test was added, showing how this feature performs for a complex object such as Conformer.

Point for discussion

Arkane's thermo and kinetics blocks we output are not objects, and have no real pythonic form. Here I added a tentative fix for them, we might want to reconsider how to output them better.
I'd like to try removing (not fixing) thermo and kinetic output for output.py, and instead (and finally) save respective RMG library files. We will still output the data in the table form for both in output.py.


Here's an example output:

# Conformer for C7H7:
{   'E0': {   'class': 'ScalarQuantity',
              'units': 'kJ/mol',
              'value': 193.74920136178187},
    'class': 'Conformer',
    'coordinates': {   'class': 'ArrayQuantity',
                       'units': 'angstroms',
                       'value': {   'class': 'np_array',
                                    'object': [   [0.0, 0.0, -1.90931],
                                                  [0.0, 1.20973, -1.20462],
                                                  [   0.0,
                                                      1.2163699999999997,
                                                      0.17817],
                                                  [0.0, 0.0, 0.92089],
                                                  [   0.0,
                                                      -1.2163699999999997,
                                                      0.17817],
                                                  [0.0, -1.20973, -1.20462],
                                                  [0.0, 0.0, -2.99302],
                                                  [0.0, 2.14897, -1.74692],
                                                  [0.0, 2.15757, 0.71765],
                                                  [0.0, -2.15757, 0.71765],
                                                  [0.0, -2.14897, -1.74692],
                                                  [0.0, 0.0, 2.32493],
                                                  [0.0, 0.92727, 2.88399],
                                                  [0.0, -0.92727, 2.88399]]}},
    'mass': {   'class': 'ArrayQuantity',
                'units': 'amu',
                'value': {   'class': 'np_array',
                             'object': [   12.000000000000002,
                                           12.000000000000002,
                                           12.000000000000002,
                                           12.000000000000002,
                                           12.000000000000002,
                                           12.000000000000002,
                                           1.00782503224,
                                           1.00782503224,
                                           1.00782503224,
                                           1.00782503224,
                                           1.00782503224,
                                           12.000000000000002,
                                           1.00782503224,
                                           1.00782503224]}},
    'modes': [   {   'class': 'IdealGasTranslation',
                     'mass': {   'class': 'ScalarQuantity',
                                 'units': 'amu',
                                 'value': 91.05478},
                     'quantum': False},
                 {   'class': 'NonlinearRotor',
                     'inertia': {   'class': 'ArrayQuantity',
                                    'units': 'amu*angstrom^2',
                                    'value': {   'class': 'np_array',
                                                 'object': [   91.05665505889641,
                                                               186.6754595575174,
                                                               277.7326559036768]}},
                     'quantum': False,
                     'rotationalConstant': {   'class': 'ArrayQuantity',
                                               'units': 'cm^-1',
                                               'value': {   'class': 'np_array',
                                                            'object': [   0.18513340999012978,
                                                                          0.09030447329984698,
                                                                          0.060697324189327787]}},
                     'symmetry': 2},
                 {   'class': 'HarmonicOscillator',
                     'frequencies': {   'class': 'ArrayQuantity',
                                        'units': 'cm^-1',
                                        'value': {   'class': 'np_array',
                                                     'object': [   199.38075265757388,
                                                                   360.5363271321874,
                                                                   413.7949151525557,
                                                                   480.34741903286914,
                                                                   536.2845084113071,
                                                                   630.7231437836404,
                                                                   687.1180869560582,
                                                                   709.613210214998,
                                                                   776.6617864901739,
                                                                   830.4036310625469,
                                                                   834.386032685694,
                                                                   901.8407934891261,
                                                                   973.4982807999646,
                                                                   975.1481503393776,
                                                                   993.3485157754649,
                                                                   998.6064486942171,
                                                                   1040.1386185625333,
                                                                   1120.6925785487256,
                                                                   1179.2234881852514,
                                                                   1189.0708173043222,
                                                                   1292.8647176431568,
                                                                   1332.909949640503,
                                                                   1357.1753414643513,
                                                                   1479.4620076104507,
                                                                   1495.3606146408765,
                                                                   1507.9144088991889,
                                                                   1583.1430987057158,
                                                                   1604.628263865009,
                                                                   3156.8525679482696,
                                                                   3170.2206892536133,
                                                                   3172.7811150780485,
                                                                   3185.0534994349905,
                                                                   3189.799070842567,
                                                                   3203.232858579444,
                                                                   3253.9889510503353]}},
                     'quantum': True},
                 {   'class': 'HinderedRotor',
                     'fourier': {   'class': 'ArrayQuantity',
                                    'units': 'kJ/mol',
                                    'value': {   'class': 'np_array',
                                                 'object': [   [   -0.31592326520854164,
                                                                   -27.766519216358134,
                                                                   0.177678139795233,
                                                                   3.2437014555229857,
                                                                   0.050951495010736314],
                                                               [   -0.0016495340958863763,
                                                                   -0.0021925030855088038,
                                                                   -0.003863963373036643,
                                                                   -0.0009120676025288776,
                                                                   0.0027420643405067606]]}},
                     'frequency': 0.0,
                     'inertia': {   'class': 'ScalarQuantity',
                                    'units': 'amu*angstrom^2',
                                    'value': 1.7001286526557986},
                     'quantum': True,
                     'rotationalConstant': {   'class': 'ScalarQuantity',
                                               'units': 'cm^-1',
                                               'value': 9.915501998636937},
                     'semiclassical': False,
                     'symmetry': 2}],
    'number': {   'class': 'ArrayQuantity',
                  'value': {   'class': 'np_array',
                               'object': [   6.0,
                                             6.0,
                                             6.0,
                                             6.0,
                                             6.0,
                                             6.0,
                                             1.0,
                                             1.0,
                                             1.0,
                                             1.0,
                                             1.0,
                                             6.0,
                                             1.0,
                                             1.0]}},
    'optical_isomers': 1,
    'spin_multiplicity': 2}

@alongd alongd requested a review from mjohnson541 October 3, 2019 02:34
@alongd alongd self-assigned this Oct 3, 2019
@codecov
Copy link

codecov bot commented Oct 3, 2019

Codecov Report

Merging #1744 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1744   +/-   ##
======================================
  Coverage    32.6%   32.6%           
======================================
  Files          87      87           
  Lines       26115   26115           
  Branches     6875    6875           
======================================
  Hits         8516    8516           
+ Misses      16641   16629   -12     
- Partials      958     970   +12
Impacted Files Coverage Δ
rmgpy/data/statmech.py 42.2% <0%> (ø) ⬆️
rmgpy/rmg/pdep.py 12.21% <0%> (ø) ⬆️
rmgpy/reaction.py 0% <0%> (ø) ⬆️
rmgpy/data/kinetics/family.py 48.28% <0%> (ø) ⬆️
rmgpy/yml.py 15.71% <0%> (ø) ⬆️
rmgpy/data/kinetics/database.py 50.61% <0%> (ø) ⬆️
rmgpy/molecule/molecule.py 0% <0%> (ø) ⬆️
rmgpy/statmech/ndTorsions.py 59.78% <0%> (ø) ⬆️

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 59c93db...3737291. Read the comment docs.

@mliu49
Copy link
Contributor

mliu49 commented Oct 3, 2019

Prettify was broken because of changes to method names in PrettifyVisitor in 59d8060. It subclasses ast.NodeVisitor, which relies on having special methods named visit_classname, where classname may be Call, Str, Num, etc.

For now, I think we should revert the method renaming in this class to restore the original behavior. However, I do agree that we should reconsider our output formats in the near future.

@alongd
Copy link
Member Author

alongd commented Oct 3, 2019

OK, I reverted the PEP8 prettify changes, and added a test.
We'll find time to discuss a new format for Arkane's output.
Thanks for the guidance, @mliu49!

@mliu49
Copy link
Contributor

mliu49 commented Oct 3, 2019

I added two more fixes and a unit test. Turns out that Python 3 handles negative numbers differently than Python 2...

Copy link
Member Author

@alongd alongd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! I added one comment/question

@alongd
Copy link
Member Author

alongd commented Oct 9, 2019

@mliu49, thanks for your excellent additions to this PR!
I rebased and squashed, should be ready to merge after the tests pass.

@mliu49
Copy link
Contributor

mliu49 commented Oct 9, 2019

Sorry, I merged another PR so I went ahead and rebased this again. I will merge after tests pass, unless you think we need a third reviewer.

@alongd alongd removed the Status: Discussion Needed Discussion needed for further progress label Oct 10, 2019
@alongd
Copy link
Member Author

alongd commented Oct 10, 2019

@mliu49, I think this is ready to be merged. Thanks again

@mliu49 mliu49 merged commit 5d7c136 into master Oct 10, 2019
@mliu49 mliu49 deleted the prettify branch October 10, 2019 12:30
@mliu49 mliu49 mentioned this pull request Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants