Improved coding style in Arkane #1596

alongd · 2019-05-09T19:39:25Z

Improved coding style in Arkane: Adhering to PEP 8 and additional minor code simplifications

codecov · 2019-05-09T20:22:18Z

Codecov Report

Merging #1596 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #1596      +/-   ##
=========================================
+ Coverage   41.58%   41.6%   +0.01%     
=========================================
  Files         176     176              
  Lines       29147   29147              
  Branches     5995    5995              
=========================================
+ Hits        12122   12127       +5     
+ Misses      16187   16183       -4     
+ Partials      838     837       -1

Impacted Files	Coverage Δ
rmgpy/data/kinetics/family.py	`52.97% <0%> (+0.23%)`	⬆️

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 fe57caa...0982f57. Read the comment docs.

mliu49

Thanks for this PR! Since the entire PR is code style changes, I made a lot of picky style comments. Since they mostly come down to personal preference, don't feel obligated to change all of them.

mliu49 · 2019-05-09T20:52:19Z

arkane/kinetics.py

@@ -609,7 +609,7 @@ def draw(self, reaction, format, path=None):
                for c in column:
                    top0 = wellRects[c][1]
                    bottom0 = top + wellRects[c][3]
-                    if (top >= top0 and top <= bottom0) or (top <= top0 and top0 <= bottom):
+                    if (bottom0 >= top >= top0) or (bottom >= top <= top0):


Would it make sense to order these as one would normally for math expressions, i.e. top0 <= top <= bottom0 or top <= top0 <= bottom?

mliu49 · 2019-05-09T20:54:52Z

arkane/input.py

-             kineticsEstimator = 'rate rules',
-             ):
+def database(thermoLibraries = None, transportLibraries = None, reactionLibraries = None, frequenciesLibraries = None,
+             kineticsFamilies = 'default', kineticsDepositories = 'default', kineticsEstimator = 'rate rules'):


Remove spaces around =?

That was done in a different commit

mliu49 · 2019-05-09T20:58:08Z

arkane/common.py

+        logging.warning('the species corresponding to ' + str(os.path.basename(path))
+                        + ' is different in energy from the lowest energy conformer by '
+                        + "%0.2f" % Vdiff + ' kJ/mol. This can cause significant errors in your computed '
+                                            'rate constants. ')


Could you update this to use str.format()?

mliu49 · 2019-05-09T21:02:46Z

arkane/common.py

-'Ds': [[281, 281.16451]], 'Rg': [[280, 280.16514]], 'Cn': [[285, 285.17712]], 'Nh': [[284, 284.17873]],
-'Fl': [[289, 289.19042]], 'Mc': [[288, 288.19274]], 'Lv': [[293, 293.20449]], 'Ts': [[292, 292.20746]],
-'Og': [[294, 294.21392]]}
+                  'He': [[3, 3.0160293201, 0.00000134], [4, 4.00260325414, 0.99999866]],


Optional style preference: For long lists/dictionaries, I like putting the first entry on the next line:

mass_by_symbol = { 'H': [], 'He': [], }

Random thought, but it seems like this should be located in rmgpy.molecule.element. No need to move it now though.

mliu49 · 2019-05-09T21:03:25Z

arkane/commonTest.py

@@ -173,7 +176,8 @@ def testPressureList(self):
        """
        Test the pressure list.
        """
-        self.assertEqual(numpy.array_equal(self.PlistValue, numpy.array([0.01, 0.1, 1, 3, 10, 100, 1000])), True, msg=None)
+        self.assertEqual(numpy.array_equal(self.PlistValue, numpy.array([0.01, 0.1, 1, 3, 10, 100, 1000])), True,
+                         msg=None)


You could just get rid of the msg argument.

mliu49 · 2019-05-10T16:57:33Z

arkane/output.py

@@ -137,27 +140,29 @@ def visit_Dict(self, node):
            return result
        else:
            # Keep elements on one line
-            result = '{{{0}}}'.format(', '.join(['{0}: {1}'.format(self.visit(key), self.visit(value)) for key, value in zip(node.keys, node.values)]))
+            result = '{{{0}}}'.format(', '.join(
+                ['{0}: {1}'.format(self.visit(key), self.visit(value)) for key, value in zip(node.keys, node.values)]))


Perhaps

result = '{{{0}}}'.format(', '.join(['{0}: {1}'.format(self.visit(key), self.visit(value)) for key, value in zip(node.keys, node.values)]))

mliu49 · 2019-05-10T16:59:20Z

arkane/pdep.py

-                transitionState.conformer.E0 = (sum([spec.conformer.E0.value_si for spec in reaction.reactants]) + reaction.kinetics.Ea.value_si,"J/mol")
+                transitionState.conformer.E0 = (sum([spec.conformer.E0.value_si
+                                                     for spec in reaction.reactants]) + reaction.kinetics.Ea.value_si,
+                                                "J/mol")


I think the sum can be kept on the first line and the line break occurs right before or after the +.

mliu49 · 2019-05-10T17:03:52Z

arkane/sensitivity.py

-                           '='*(max_label-10)))
+                sa_f.write(
+                    '========================={0}==================================================\n\n\n'.format(
+                        '=' * (max_label - 10)))


You could consider splitting the string before \n to put this on two lines.

mliu49 · 2019-05-10T17:05:26Z

arkane/statmech.py

+            # Note: If your model chemistry does not include spin orbit coupling, you should add the corrections
+            # to the energies here
+            if modelChemistry.startswith(
+                    'cbs-qb3'):  # only check start of string to allow different bond corrections (see below)


I would prefer putting the comment on the following line instead of splitting the code.

mliu49 · 2019-05-10T17:08:57Z

arkane/kinetics.py

@@ -284,8 +284,8 @@ def save(self, outputFile):
            f.write('# krev (TST+T) = {0} \n\n'.format(kineticsrev))

        # Reaction path degeneracy is INCLUDED in the kinetics itself!
-        string = 'kinetics(label={0!r}, kinetics={1!r})'.format(reaction.label, reaction.kinetics)
-        f.write('{0}\n\n'.format(prettify(string)))
+        _string = 'kinetics(label={0!r}, kinetics={1!r})'.format(reaction.label, reaction.kinetics)


I would probably name it something more informative, like rxn_str or kinetics_str.

alongd · 2019-05-10T18:54:00Z

Thanks!
I squashed the fixups for _string and simplifying the chained comparison (x <= y <= z). Everything else is still in fixup commits

mliu49 · 2019-05-13T15:30:58Z

arkane/main.py

-        parser.add_argument('file', metavar='FILE', type=str, nargs=1,
-                            help='a file describing the job to execute')
+        parser = argparse.ArgumentParser(description="""Arkane is a Python toolkit for computing chemical reaction 
+        rates and other properties used in detailed kinetics models using various methodologies and theories.""")


Sorry, my previous comment wasn't very clear. I was referring to the opening quotes """ only. I think the line breaks in the text are likely intentional and should be preserved, since they transfer directly to the output in terminal.

mliu49 · 2019-05-13T15:33:19Z

Thanks for making all of those fixes! You can go ahead and squash, including whatever change you make regarding my new comment.

to avoid shadowing reserved word

Also replaced assertEqual(x, True) with assertTrue(x)

alongd · 2019-05-13T19:20:40Z

@mliu49, I think this PR is ready to go

Style: Simplified boolean expressions

37534fc

alongd requested a review from mliu49 May 9, 2019 19:39

alongd self-assigned this May 9, 2019

mliu49 reviewed May 10, 2019

View reviewed changes

alongd added 5 commits May 10, 2019 14:14

Style: Simplified chained comparisons in Arkane kinetics.py

d912cd2

Style: Define instance attributes inside __init__ in Arkane explorer.py

0c4ef2d

Style: Changed first parameter in test setUp/tearDown to cls

f71b473

Added __init__ call of super class for ArkaneSpecies

6640fc8

Added various missing docstrings in Arkane

9b225c3

alongd force-pushed the arkane_style branch from b8b4962 to c680b82 Compare May 10, 2019 18:52

mliu49 reviewed May 13, 2019

View reviewed changes

alongd added 16 commits May 13, 2019 12:53

PEP 8 coding style fixes in Arkane common.py

7e88460

PEP 8 coding style fixes in Arkane commonTest.py

602d185

PEP 8 coding style fixes in Arkane explorer.py

553d204

PEP 8 coding style fixes in Arkane explorerTest.py

a81eb93

PEP 8 coding style fixes in Arkane gaussian.py

4519edd

PEP 8 coding style fixes in Arkane gaussianTest.py

c27ee75

PEP 8 coding style fixes in Arkane input.py

6e08c91

PEP 8 coding style fixes in Arkane kinetics.py

e2303b5

PEP 8 coding style fixes in Arkane log.py

01e944f

PEP 8 coding style fixes in Arkane main.py

66e42ef

PEP 8 coding style fixes in Arkane output.py

a2fb559

PEP 8 coding style fixes in Arkane pdep.py

064ce44

PEP 8 coding style fixes in Arkane pdepTest.py

11a7156

PEP 8 coding style fixes in Arkane qchem.py

9a87780

PEP 8 coding style fixes in Arkane qchemTest.py

afb051b

PEP 8 coding style fixes in Arkane sensitivity.py

8eaf944

alongd added 6 commits May 13, 2019 12:55

PEP 8 coding style fixes in Arkane statmech.py

c415475

PEP 8 coding style fixes in Arkane thermo.py

e0a88dd

Style: Removed redundant paranthesis in Arkane statmech.py

ec04139

Minor: Renamed 'string' as 'rxn_str'

41e1913

to avoid shadowing reserved word

Changed warning in Arkane common.py to the string.format method

7a34b5a

Arkane common test: Removed msg=None

0982f57

Also replaced assertEqual(x, True) with assertTrue(x)

alongd force-pushed the arkane_style branch from c680b82 to 0982f57 Compare May 13, 2019 16:56

mliu49 approved these changes May 13, 2019

View reviewed changes

mliu49 merged commit baed747 into master May 13, 2019

mliu49 deleted the arkane_style branch May 13, 2019 21:14

mliu49 mentioned this pull request May 15, 2019

RMG v2.4.0 (?) Release Planning #1591

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved coding style in Arkane #1596

Improved coding style in Arkane #1596

alongd commented May 9, 2019

codecov bot commented May 9, 2019 •

edited

mliu49 left a comment

mliu49 May 9, 2019

mliu49 May 9, 2019

alongd May 10, 2019

mliu49 May 9, 2019

mliu49 May 9, 2019

mliu49 May 9, 2019

mliu49 May 10, 2019

mliu49 May 10, 2019

mliu49 May 10, 2019

mliu49 May 10, 2019

mliu49 May 10, 2019

alongd commented May 10, 2019

mliu49 May 13, 2019

mliu49 commented May 13, 2019

alongd commented May 13, 2019

Improved coding style in Arkane #1596

Improved coding style in Arkane #1596

Conversation

alongd commented May 9, 2019

codecov bot commented May 9, 2019 • edited

Codecov Report

mliu49 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd commented May 10, 2019

Choose a reason for hiding this comment

mliu49 commented May 13, 2019

alongd commented May 13, 2019

codecov bot commented May 9, 2019 •

edited