-
Notifications
You must be signed in to change notification settings - Fork 230
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
Created a new reactor type MBSampledReactor for modeling laser lab Mass-Spec experiments #1669
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1669 +/- ##
==========================================
+ Coverage 41.75% 41.87% +0.11%
==========================================
Files 176 177 +1
Lines 29423 29490 +67
Branches 6075 6097 +22
==========================================
+ Hits 12286 12349 +63
- Misses 16261 16262 +1
- Partials 876 879 +3
Continue to review full report at Codecov.
|
I think it would make sense for MBSampledReactor to be a subclass of SimpleReactor. I compared the two, and it seemed like only the I added a commit making this change, but have not tested it yet beyond checking that it compiled correctly. Could you run a simulation to see if this works and gives the same result as before? |
Update regarding my previous commit. Turns out that it won't work because SimpleReactor defines a |
40ff482
to
0bccf72
Compare
For modeling molecular beam sampling of species from an isothermal, isobaric, homogenous batch reactor. The sampling process is modeled as two first-order unimolecular reactions, following the approach developed by Baeza-Romero et al., in their 2011 IJCK paper. Currently, the new reactor is only intended for use with the simulate.py script, after a conventional RMG mechanism has already been made.
0bccf72
to
42dae5e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have looked over the unit test portions as requested. There is just one quick fix, but otherwise this looks good. I am about to test that the tests run properly by running locally, but go ahead and rebase with the fix squashed.
2b2fe62
to
206f149
Compare
Yeah, the concentrations are all extremely low, so they don't show up on the plot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unit test commits are approved as far as I am concerned. If the reactor commit is fine with you then this PR can be merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To document offline discussion, we decided that even though the MBSampledReactor is not up to date with all of the changes to SimpleReactor, it's ok because it does not need to be used in RMG jobs. The workflow currently used is to generate an RMG model using SimpleReactor and then use MBSampledReactor to simulate and compare to the experiments.
Motivation or Problem
To model the sampling delay in mass-spec of Green group laser apparatus, it is important to generate a new reactor type.
Description of Changes
New reactor type added, and can be used in simulate.py
Testing
The MBSampledReactor added works same as what it was before rebase on master. Same time-dependent concentration for species by giving same input files
Reviewer Tips