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

Update IPython notebooks #1735

Merged
merged 9 commits into from Oct 26, 2019
Merged

Update IPython notebooks #1735

merged 9 commits into from Oct 26, 2019

Conversation

mliu49
Copy link
Contributor

@mliu49 mliu49 commented Sep 30, 2019

Motivation or Problem

This PR updates the notebooks located in RMG-Py/ipython to work with Python 3 and RMG API changes.

The only exception is globalUncertainty.ipynb, which will be updated along with the MUQ changes in a separate PR.

Description of Changes

In the process of updating the notebooks, a few bugs were also discovered and fixed.

The parse_source example model also had to be updated to reflect RMG-database changes (in particular, the replacement of the R_Recombination tree with auto-generated groups).

Testing

I have run all of the notebooks locally. To confirm, the reviewer can do the same. All of the notebooks can be executed in their entirety without changing any user inputs, and should run without any errors.

@mliu49 mliu49 added the Status: Ready for Review PR is complete and ready to be reviewed label Sep 30, 2019
@mliu49 mliu49 self-assigned this Sep 30, 2019
@codecov
Copy link

codecov bot commented Sep 30, 2019

Codecov Report

Merging #1735 into master will increase coverage by <.01%.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1735      +/-   ##
==========================================
+ Coverage    32.6%   32.61%   +<.01%     
==========================================
  Files          87       87              
  Lines       26116    26124       +8     
  Branches     6875     6878       +3     
==========================================
+ Hits         8516     8521       +5     
+ Misses      16642    16633       -9     
- Partials      958      970      +12
Impacted Files Coverage Δ
rmgpy/molecule/translator.py 0% <0%> (ø) ⬆️
rmgpy/molecule/draw.py 40.05% <0%> (-0.13%) ⬇️
rmgpy/data/kinetics/rules.py 51.59% <100%> (+0.27%) ⬆️
rmgpy/data/kinetics/family.py 48.35% <63.63%> (+0.06%) ⬆️
rmgpy/data/statmech.py 42.2% <0%> (ø) ⬆️
rmgpy/rmg/pdep.py 12.21% <0%> (ø) ⬆️
rmgpy/reaction.py 0% <0%> (ø) ⬆️
rmgpy/yml.py 15.71% <0%> (ø) ⬆️
rmgpy/data/kinetics/database.py 50.61% <0%> (ø) ⬆️
... and 2 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 7147277...8a057a4. Read the comment docs.

@mliu49
Copy link
Contributor Author

mliu49 commented Oct 7, 2019

@mjohnson541 and @alongd, it would be really nice to get this in before the IPython session on Wednesday. Do you have time to look at it or should I request other reviewers?

@alongd
Copy link
Member

alongd commented Oct 8, 2019

I'm sorry, I won't have time for a decent review by Wednesday.

Copy link
Member

@amarkpayne amarkpayne left a comment

Choose a reason for hiding this comment

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

Just a couple of quick comments, but everything looks good overall so far. I want to spend some time running the notebooks though just to make sure that they run properly now

" },\n",
" terminationTime=('''+str(sim_time/1000.0)+''','s'),\n",
" }},\n",
" terminationTime=({sim_time/1000.0!s},'s'),\n",
Copy link
Member

Choose a reason for hiding this comment

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

The !s formatting might not be necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this specific one or in general?

Copy link
Member

Choose a reason for hiding this comment

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

I think in general for the cases I saw in this file

ipython/findParameterSourcesAndAssignUncertainies.ipynb Outdated Show resolved Hide resolved
rmgpy/data/kinetics/family.py Outdated Show resolved Hide resolved
ipython/mechanism_analyzer.ipynb Outdated Show resolved Hide resolved
@amarkpayne
Copy link
Member

amarkpayne commented Oct 17, 2019

Quick comment on running the IPython notebooks: uncommenting the line job.load_chemkin_model('data/minimal_model/chem_annotated.inp',transport_file='data/minimal_model/tran.dat') in the 5th code cell of cantera_sensitivity_comparison is broken. It is complaining about a missing file, did it get renamed? Perhaps it is now the "ethane_model"?

@amarkpayne
Copy link
Member

I think this PR is good to go. Rebase to update this and I'll merge it in

It was leading to circular imports, and was only used for
an isinstance check, which was refactored
Main motivation is that None can no longer be compared to ints in
Python 3

This changes how the sorting process is implemented but maintains
the same behavior as before
In cases where there may have been a line break in the middle of
the reaction template, which is very likely with automatically
generated families.
The R_Recombination family was changed for auto tree generation
There is only a single template now, and duplicate * labels
This copies template handling from __generate_reactions
A unit test is also added
@amarkpayne amarkpayne merged commit f70d761 into master Oct 26, 2019
@amarkpayne amarkpayne deleted the ipython branch October 26, 2019 02:26
@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
Labels
Status: Ready for Review PR is complete and ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants