ABC Simulator #13

kspieks · 2020-04-18T19:14:55Z

This PR creates a template for adding SA adapters. It implements SA with RMG. I welcome all feedback and look forward to your comments :)

kspieks · 2020-04-18T19:30:25Z

Hmm the Travis tests failed due to an import error from RMG-Py even though the master branch of RMG-Py passes all tests. I also tried running coverage run -m pytest -ra, which is the same command travis uses, and it passed locally for me giving the following output

coverage run -m pytest -ra
============================================================ test session starts ============================================================
platform darwin -- Python 3.7.6, pytest-5.4.1, py-1.8.1, pluggy-0.13.1
rootdir: /Users/kevin/Documents/Green_Lab/RMG/T3, inifile: pytest.ini
collected 18 items                                                                                                                          

tests/test_main.py ................                                                                                                   [ 88%]
tests/test_utils.py ..                                                                                                                [100%]

============================================================ 18 passed in 13.31s ============================================================

I must be missing something obvious. Can you help me understand why these imports are failing?

alongd · 2020-04-18T21:34:55Z

T3 uses RMG's binaries from conda...
For now, we can import RMG's master for the tests. Let's make a separate PR for that

kspieks · 2020-04-18T21:47:19Z

T3 uses RMS's binaries from conda...
For now we can import RMG's master for the tests. Let's make a separate PR for that

Sorry what files must we update in a separate PR? I'm not seeing where the RMS binaries are specified.

alongd · 2020-04-19T13:52:15Z

I had a typo, it's RMG, not RMS. I'll make the PR

kspieks · 2020-04-19T14:15:32Z

I had a typo, it's RMG, not RMS. I'll make the PR

Okay thanks for making the PR. Admittedly I'm still confused where RMG's conda binaries entered the travis build so I'll learn from your PR

kspieks · 2020-04-22T00:14:34Z

@alongd Can you review this PR when you get a chance?

alongd

Thanks for this addition, introducing ABC into T3!!
I added some comments, most are minor.

tandem/sa/adapter.py

tandem/sa/factory.py

alongd · 2020-04-22T01:37:12Z

tandem/sa/factory.py

+               run_directory: str,
+               input_file: str,
+               threshold: Optional[float] = 0.001,
+               verbose: Optional[bool] = True,


also need to add SA tolerances, see http://reactionmechanismgenerator.github.io/RMG-Py/users/rmg/input.html#simulator-tolerances

tandem/sa/__init__.py

tandem/sa/rmg_sa.py

tests/test_rmg_sa.py

alongd · 2020-04-22T15:32:37Z

Let's asses what parameters RMS/Cantera require, and make sure we have them in out SAAdapter class. I think the most complicated thing is the reactor type: for RMG we simply use the reactor type from the input file, but here we should let the user specify how to simulate and run SA.

xiaoruiDong · 2020-04-22T17:27:17Z

Thank you for the PR. I totally support the transition to adapters. However, since sensitivity is also a simulation just with more things to calculate, I don't know if you agree that we should have a simulation class (adapter) first and build SA adapter on that.

Below is a link a cantera sensitivity input example:
https://projects.skill-lync.com/projects/Sensitivity-Analysis-of-GRI30-Mechanism-using-Cantera-36940
To me, the simulation input is just like a subset to the sensitivity input.

Besides, I saw there is an argument (input_file), do we want to pursue a workflow of writing the input file first and then apply it to this SA factory, or it is just the input_file of RMG we used to generate the model?

If the first case, the observable list can be removed (included in the input file). I think it introduces more flexibility. For example, I use range reactor in the model generation, while using a range of different conditions to do sensitivities. Besides, adiabatic conditions are also something I considered for sensitivity. Besides, in combustion community, sensitivity analysis on ignition delay is another thing wanted. This is probably also something we want to add into consideration. So, since we want to build input_files, we need to have a template write. (Jinja2 is something we want to pursue, I guess. Its inheritance function is useful).

If the second case, I feel that we will be limited, but probably let me know your reason first.

alongd · 2020-04-22T17:47:42Z

good point @xiaoruiDong, let's make a SimulateAdapter instead, we could have a bool flag for requesting SA in addition.

Regarding the other points, we're just starting to discuss how to use Cantera and RMS, this part isn't ripe yet. Yes, it's possible to ask the user to supply an additional script for the simulation, specifying the reactors.

kspieks · 2020-04-22T20:51:38Z

Thank you both for this valuable feedback! I agree that the first case you describe offers better generalizability. I'll start working on a new branch called abc simulate that tries to incorporate these ideas: creates a simulate class that uses an input file separate from the RMG input.py and ARC arc.yml files passed to T3

I have some clarifying questions for you both:

So the big idea is that once T3 has completed model generation, we allow the user to simulate the model with either RMG, RMS, or Cantera? As an additional feature, the user can also specify which software to use when performing SA?
What motivates performing SA for different T, P, or in a different reactor type than what was used to generate the model? I didn't know this was a common need, which is why I hadn't planned for it when writing the SA adapter for this PR.

kspieks · 2020-04-22T20:59:52Z

I was in the process of addressing Alon's comments so I just pushed the changes for all resolved conversations

alongd · 2020-04-22T21:02:05Z

I think we don't necessarily need a new branch.
In the grand scheme of things, T3 will also be able to generate a report with fluxes, profiles, etc.:

Since RMG has limited reactor types (which is OK for model generation), the process being modeled might be different and require a more complicated reactor model.

kspieks · 2020-04-22T21:08:15Z

I think we don't necessarily need a new branch.
In the grand scheme of things, T3 will also be able to generate a report with fluxes, profiles, etc.:

Since RMG has limited reactor types (which is OK for model generation), the process being modeled might be different and require a more complicated reactor model.

I see. I was unaware that developing a model in reactor A and then simulating in reactor B gave good results. Good to know!

Okay I can keep this branch. I was considering creating a new Simulate class that has a method for performing SA. But I guess is it better organization to have both Simulate and SA be separate classes?

alongd · 2020-04-22T21:17:03Z

I agree with the first approach. We could have one (ABC) class, say SimulateAdaptor, that has many methods such as SA, UA, species profiles, fluxes... and at the core of all these features (I think) is simulating the reactor(s).

kspieks · 2020-04-22T21:40:37Z

I agree with the first approach. We could have one (ABC) class, say SimulateAdaptor, that has many methods such as SA, UA, species profiles, fluxes... and at the core of all these features (I think) is simulating the reactor(s).

I see. Thanks for clarifying. So would simulate just be an additional method that SA, UA, etc would call?

As another question, how much flexibility are we looking for? Would SA, UA, etc actually be abstract methods in an ABC SimulateAdapter? Say we have 3 adapters: RMG, RMS, Cantera. Each has methods for simualte, SA, UA, etc. Normally, we would use the factory to create one instance of the needed adapter. I'm thinking it would be cumbersome for a user to simulate with Cantera and do SA with RMS with this set up. But perhaps this isn't necessary?

codecov-io · 2020-04-22T21:47:13Z

Codecov Report

Merging #13 into master will increase coverage by 5.28%.
The diff coverage is 61.05%.

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
+ Coverage   44.18%   49.46%   +5.28%     
==========================================
  Files           4        8       +4     
  Lines         817      849      +32     
  Branches      238      238              
==========================================
+ Hits          361      420      +59     
+ Misses        400      363      -37     
- Partials       56       66      +10

Impacted Files	Coverage Δ
tandem/main.py	`50.23% <33.33%> (+3.80%)`	⬆️
tandem/sa/factory.py	`50.00% <50.00%> (ø)`
tandem/sa/rmg_sa.py	`61.64% <61.64%> (ø)`
tandem/sa/adapter.py	`75.00% <75.00%> (ø)`
tandem/sa/__init__.py	`100.00% <100.00%> (ø)`
tandem/logger.py	`25.96% <0.00%> (+5.76%)`	⬆️

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 1b9fd37...6588b14. Read the comment docs.

kspieks · 2020-05-26T22:24:43Z

@alongd Can you take a look at this draft of the RMGSimulator when you get a chance?

alongd

Looking good! I added a minor comment, let's work on the input file PR (I'll push soon) so we'll have the correct inputs for these adapters

alongd · 2020-05-27T11:26:08Z

tandem/simulate/rmg_simulator.py

+        sa_rtol (Optional[float]): The relative tolerance used when running SA.
+        verbose (Optional[bool]): Whether or not to log to file.
+
+    ======================= ======      ====================================================


we don't use this style for attributes in T3 (we do in RMG), you can have an Attributes: section in a similar format to Args:

Thanks for reviewing! I just pushed the updated format for documenting the attributes. I think the pyrms simulator adapter is ready as well, just needs SA to be working in rms. I'm happy to help with the T3 input file in the meantime :)

alongd · 2020-05-27T11:27:19Z

tandem/simulate/rmg_simulator.py

+
+        Returns:
+             A SA dictionary, whose structure is given below.
+             sa_dict = { 'thermo' :


excellent, thanks!

Run SA using the simulate adapter rather than the previous `run_sa()` method. Remove code from `run_sa()` that is now part of the `get_sa_coefficients()` method in the RMG adapter. Remove the corresponding imports in main.py that have now been moved to the RMG Simulator Adapter.

Update environment.yml to include pyrms and Matt's channel Add scripts to organize the installation of ARC, Julia 1.4, RMS, and pycall. Note: the pyrms conda package is currently for v0.2.1. SA is functional only on v0.3.0 or later and we must use RMS v0.3.2 to fix the constantPDomain error

alongd · 2021-03-12T07:29:40Z

@kspieks, everything looks good to me. Are we good to merge this in?

kspieks · 2021-03-12T13:06:18Z

@alongd Yes I believe so. Thanks again for all of your helpful feedback in developing this PR. I'm happy to look at PR #69 after this is merged in as well as discuss other next steps

alongd

@kspieks, Thank you. This is a well deserved merge: a seminal PR which reflects a significant improvement in T3's abilities and flexibility. Thanks for the professional work! 🥇

kspieks requested a review from alongd April 18, 2020 19:14

kspieks self-assigned this Apr 18, 2020

kspieks force-pushed the abc_sa branch from 969ae69 to dcc7624 Compare April 21, 2020 16:05

alongd reviewed Apr 22, 2020

View reviewed changes

kspieks force-pushed the abc_sa branch from dcc7624 to f2ed5b9 Compare April 22, 2020 20:59

kspieks force-pushed the abc_sa branch from f2ed5b9 to 6588b14 Compare April 22, 2020 21:35

kspieks added the WIP This is currently work-in-progress label May 5, 2020

kspieks changed the title ~~Abc sa~~ [WIP]: Abc sa May 5, 2020

kspieks force-pushed the abc_sa branch from 6588b14 to 1c53613 Compare May 26, 2020 22:23

alongd reviewed May 27, 2020

View reviewed changes

kspieks force-pushed the abc_sa branch from 1c53613 to 0e67b16 Compare May 27, 2020 14:17

kspieks force-pushed the abc_sa branch from 0e67b16 to 2971284 Compare June 23, 2020 18:50

ReactionMechanismGenerator deleted a comment from lgtm-com bot Jun 23, 2020

kspieks added 24 commits March 9, 2021 13:30

Add adapter for simulating with RMG

0e4e4c6

Tests: Added RMG Simulator Adapter tests

fba4bcd

Tests: Modify test_main to account for simulate adapter

afe2207

Update minimal_data to reflect new directory structure with adapters

580366c

Add validator to ensure RMG options block is filled with default values

254e0e5

Tests: Update test_writer to account for RMG options validator

5064229

Add RMS adapters

3d0e1e2

Tests: Add tests for RMS adapter

a867f71

Add function that converts termination time to seconds for RMS

cb0e63b

Initialize imports for the simulator modules

bc639f3

Reorganize simulate tests

e3953c8

Add Cantera adapter

d9e8538

Tests: Add tests for Cantera adapter

ae40a1f

Minor: consolidate scripts so installation is one step

02edf43

Add notebooks showing how to use simulator adapters

9e69a0b

Minor: use consistent keys for sa_dict_max and sa_dict

e35f304

Minor: add ignore_errors=True to all shutil.rmtree calls

a1f2571

Use new make install command

59a5829

Docs: add a tutorial for adding a new simulate adapter

1bc385c

Minor: update default sensitivity adapter to be RMGSimulatorConstantTP

aebe84e

Tests: ensure that the simulate adapter string is properly registered

25aab1a

Minor: Update simulate adapter names

3b0e16c

kspieks force-pushed the abc_sa branch from db39c40 to 3b0e16c Compare March 9, 2021 18:31

alongd approved these changes Mar 12, 2021

View reviewed changes

alongd merged commit 2fa9199 into master Mar 12, 2021

alongd deleted the abc_sa branch March 12, 2021 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ABC Simulator #13

ABC Simulator #13

kspieks commented Apr 18, 2020 •

edited

Loading

kspieks commented Apr 18, 2020 •

edited

Loading

alongd commented Apr 18, 2020 •

edited

Loading

kspieks commented Apr 18, 2020

alongd commented Apr 19, 2020

kspieks commented Apr 19, 2020

kspieks commented Apr 22, 2020

alongd left a comment

alongd Apr 22, 2020

alongd commented Apr 22, 2020

xiaoruiDong commented Apr 22, 2020 •

edited

Loading

alongd commented Apr 22, 2020

kspieks commented Apr 22, 2020

kspieks commented Apr 22, 2020 •

edited

Loading

alongd commented Apr 22, 2020

kspieks commented Apr 22, 2020

alongd commented Apr 22, 2020

kspieks commented Apr 22, 2020

codecov-io commented Apr 22, 2020 •

edited

Loading

kspieks commented May 26, 2020

alongd left a comment

alongd May 27, 2020

kspieks May 27, 2020

alongd May 27, 2020

alongd commented Mar 12, 2021

kspieks commented Mar 12, 2021

alongd left a comment

ABC Simulator #13

ABC Simulator #13

Conversation

kspieks commented Apr 18, 2020 • edited Loading

kspieks commented Apr 18, 2020 • edited Loading

alongd commented Apr 18, 2020 • edited Loading

kspieks commented Apr 18, 2020

alongd commented Apr 19, 2020

kspieks commented Apr 19, 2020

kspieks commented Apr 22, 2020

alongd left a comment

Choose a reason for hiding this comment

alongd Apr 22, 2020

Choose a reason for hiding this comment

alongd commented Apr 22, 2020

xiaoruiDong commented Apr 22, 2020 • edited Loading

alongd commented Apr 22, 2020

kspieks commented Apr 22, 2020

kspieks commented Apr 22, 2020 • edited Loading

alongd commented Apr 22, 2020

kspieks commented Apr 22, 2020

alongd commented Apr 22, 2020

kspieks commented Apr 22, 2020

codecov-io commented Apr 22, 2020 • edited Loading

Codecov Report

kspieks commented May 26, 2020

alongd left a comment

Choose a reason for hiding this comment

alongd May 27, 2020

Choose a reason for hiding this comment

kspieks May 27, 2020

Choose a reason for hiding this comment

alongd May 27, 2020

Choose a reason for hiding this comment

alongd commented Mar 12, 2021

kspieks commented Mar 12, 2021

alongd left a comment

Choose a reason for hiding this comment

kspieks commented Apr 18, 2020 •

edited

Loading

kspieks commented Apr 18, 2020 •

edited

Loading

alongd commented Apr 18, 2020 •

edited

Loading

xiaoruiDong commented Apr 22, 2020 •

edited

Loading

kspieks commented Apr 22, 2020 •

edited

Loading

codecov-io commented Apr 22, 2020 •

edited

Loading