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

Python3 Script check #1840

Merged
merged 7 commits into from Dec 9, 2019
Merged

Python3 Script check #1840

merged 7 commits into from Dec 9, 2019

Conversation

yunsiechung
Copy link
Contributor

@yunsiechung yunsiechung commented Dec 3, 2019

Motivation or Problem

I tested all externally-used scripts in python 3 version RMG. This is related to the issue #1726.

  • One thing I could not check is running semi-empirical QM job using thermoEstimator.py in examples/scripts/thermoEstimator/run_QM.sh because I don't have the mopac license. Since it only throws an error that I don't have the mopac license, I don't expect thermoEstimator.py to have any error.

The internally-used scripts like generateTree.py, machineWriteDatabase.py, and rmg2to3.py are not checked in this test.

Description of Changes

  • All the scripts ran fine in python 3, but some were missing examples or had minor error with example files, so I added / fixed the examples.
  • isotopes.py does not have an example because running this example takes too much time. This script seems to run fine when I checked it.
  • convertFAME.py is deleted because it is no longer used.
  • reduction example folder is deleted because it uses the script that is no longer available.
  • One major change is that all the examples relevant to scripts are moved under examples/scripts.

Testing

I ran all the script example bash scripts and everything ran successfully.

Reviewer Tips

You can run bash scripts in examples/scripts to check that everything works fine

@yunsiechung
Copy link
Contributor Author

travis-ci/push failed on the following:

A general test for a PDep job in Arkane ... FAIL

======================================================================

FAIL: A general test for a PDep job in Arkane

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

Traceback (most recent call last):

  File "/home/travis/build/ReactionMechanismGenerator/RMG-Py/arkane/pdepTest.py", line 107, in test_pdep_job

    self.assertAlmostEquals(rxn.kinetics.get_rate_coefficient(1000.0, 1.0), 88.88253229631246)

AssertionError: 88.9961408998604 != 88.88253229631246 within 7 places (0.11360860354794511 difference)

This does not seem to be related to any of my code changes, and I'm not sure what's causing this

@mliu49
Copy link
Contributor

mliu49 commented Dec 3, 2019

I found the related issue: #1682

It seems like we never figured out the root cause, but it went away on its own in each case.

@codecov
Copy link

codecov bot commented Dec 5, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1840   +/-   ##
======================================
  Coverage    44.2%   44.2%           
======================================
  Files          83      83           
  Lines       21533   21533           
  Branches     5645    5645           
======================================
  Hits         9519    9519           
+ Misses      10961   10947   -14     
- Partials     1053    1067   +14
Impacted Files Coverage Δ
arkane/kinetics.py 12.14% <0%> (ø) ⬆️
rmgpy/data/statmech.py 42.2% <0%> (ø) ⬆️
rmgpy/rmg/pdep.py 12.21% <0%> (ø) ⬆️
rmgpy/data/kinetics/family.py 49.06% <0%> (ø) ⬆️
rmgpy/statmech/ndTorsions.py 59.78% <0%> (ø) ⬆️
rmgpy/yml.py 15.71% <0%> (ø) ⬆️
arkane/sensitivity.py 10% <0%> (ø) ⬆️
rmgpy/data/kinetics/database.py 50.61% <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 17bfb68...71f10d7. Read the comment docs.

@yunsiechung
Copy link
Contributor Author

I force-pushed this again after rebasing to official/master and got the same error as before with travis-ci/push

Reduction example folder is deleted because /rmgpy/reduction/main.py
no longer exists.
convertFAME.py is also deleted because it is no longer used
All the examples related to scripts are moved under a new folder
example/scripts. Each bash scripts are changed appropriately to have
correct relative paths for each script file.

Also, for the examples file that were throwing error, I made somes
changes to make them work:

1. generateReactions
- This example was origianlly throwing error because surface chemistry
was included in the kinetic_family in the input file. The kinetic family
is changed to 'default', so it no longer throws error

2. simulate/withoutSensitivity
- This example had minor error with default temperature range in
chem.inp file. The temperature range in chem.inp file is changed
appropriately.

3. thermoEstimator
- input.py (run.sh) example had QM on in input file when it was not
supposed to. The QM thermoEstimator has its own example (input_QM.py).
The quantumMechanics portion in the input.py is deleted.
An example for scripts/generateChemkimHTML.py is added
An example for scripts/mergeModels.py is added.
The run.sh file contains two examples: merging 2 models and merging 3
models.
An example for scripts/generateFluxDiagram.py is added
An exmaple for scripts/standardizeModelSpeceisNames.py is added
A few more lines are added in the description
Copy link
Contributor

@mliu49 mliu49 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mliu49 mliu49 merged commit c46befa into master Dec 9, 2019
@mliu49 mliu49 deleted the python3_script_check branch December 9, 2019 23:36
This was referenced Dec 13, 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.

None yet

2 participants