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

Restart attribute error #1847

Merged
merged 3 commits into from
Dec 11, 2019
Merged

Restart attribute error #1847

merged 3 commits into from
Dec 11, 2019

Conversation

amarkpayne
Copy link
Member

Motivation or Problem

See #1846

Description of Changes

This PR address the issue identified in #1846, as well as dealing with a deprecation warnings we were getting from h5py. Finally, this PR adds functional tests that run restart jobs (both with and without filters) to try to catch errors like this in the future.

Testing

The functional tests added should catch the error this PR addresses

Additional note

The functional tests simply check that the restart jobs can complete. A more robust test would be ensure that the same model is achieved, but this would require running a full RMG job, and then restarting from an intermediate seed. For the sake of time in running our tests, this was not implemented.

On my computer, the whole suite of tests in mainTest.py including the new functional tests can run in less than 150 seconds, so I don't think this will drastically increase our testing time.

@codecov
Copy link

codecov bot commented Dec 10, 2019

Codecov Report

Merging #1847 into master will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1847      +/-   ##
==========================================
+ Coverage   44.22%   44.22%   +<.01%     
==========================================
  Files          83       83              
  Lines       21539    21538       -1     
  Branches     5644     5645       +1     
==========================================
  Hits         9526     9526              
+ Misses      10957    10942      -15     
- Partials     1056     1070      +14
Impacted Files Coverage Δ
rmgpy/rmg/main.py 22.14% <0%> (+0.01%) ⬆️
rmgpy/data/statmech.py 42.2% <0%> (ø) ⬆️
rmgpy/rmg/pdep.py 12.21% <0%> (ø) ⬆️
arkane/kinetics.py 12.14% <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 cafae8d...e14e85a. Read the comment docs.

cls.rmg = RMG(input_file=os.path.join(cls.testDir, 'restart_no_filters.py'),
output_directory=os.path.join(cls.outputDir))

cls.rmg.execute()
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like executing the job should be part of the test method rather than in setup.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. Sorry, I copied from the tests above, but I agree this makes more sense.

@amarkpayne amarkpayne force-pushed the restart_attribute_error branch from 38b09c5 to d50cf62 Compare December 11, 2019 19:02
The first test restarts the minimal example after ~20 species and includes filters. The second test restarts the super-minimal example after ~9 species and does not include filters.
@amarkpayne amarkpayne force-pushed the restart_attribute_error branch from d50cf62 to e14e85a Compare December 11, 2019 19:06
@mliu49 mliu49 merged commit 9647a92 into master Dec 11, 2019
@mliu49 mliu49 deleted the restart_attribute_error branch December 11, 2019 21:18
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.

2 participants