Automatic Network Generation 3 by mjohnson541 · Pull Request #1647 · ReactionMechanismGenerator/RMG-Py

mjohnson541 · 2019-07-09T20:03:23Z

This adds a number of miscellaneous bug fixes and improvements for automatic network generation. Forbidden structure checking is now done on the edge for automatic network generation, flux reduction is now done on netReactions instead of pathReactions and has an additional very robust fallback algorithm just using rate coefficients. Fixed a bug related to reduction when an isomer is removed.

codecov · 2019-07-09T22:50:20Z

Codecov Report

Merging #1647 into master will decrease coverage by 0.09%.
The diff coverage is 6.45%.

@@            Coverage Diff            @@
##           master    #1647     +/-   ##
=========================================
- Coverage   41.87%   41.77%   -0.1%     
=========================================
  Files         176      176             
  Lines       29369    29423     +54     
  Branches     6059     6075     +16     
=========================================
- Hits        12297    12291      -6     
- Misses      16192    16257     +65     
+ Partials      880      875      -5

Impacted Files	Coverage Δ
rmgpy/rmg/pdep.py	`12.33% <6.45%> (-3.8%)`	⬇️
rmgpy/data/kinetics/family.py	`52.9% <0%> (+0.23%)`	⬆️

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 589ba9a...48f12c8. Read the comment docs.

alongd

Sorry for the delay in reviewing, please see some comments.
How would you recommend me to test these additions?

alongd · 2019-07-18T16:54:25Z

arkane/explorer.py

+                            if forbiddenStructures.isMoleculeForbidden(spc.molecule[0]):
+                                reaction_model.removeSpeciesFromEdge(reaction_model.reactionSystems, spc)
+                                reaction_model.removeEmptyPdepNetworks()
+                                logging.error(spc.label)


Is this a leftover from debugging? If not, consider adding a message?

alongd · 2019-07-18T16:57:00Z

arkane/explorer.py

+                            logging.info('adding new isomer {0} to network'.format(spc))
+                            flags = np.array([s.molecule[0].getFormula() == form for s in reaction_model.core.species])
+                            reaction_model.enlarge((network, spc), reactEdge=False, unimolecularReact=flags,
                                                       bimolecularReact=np.zeros((len(reaction_model.core.species),


fix the over-indentation?

alongd · 2019-07-18T16:59:04Z

rmgpy/rmg/pdep.py


+        nrxns = []
+        for nrxn in self.netReactions:
+           prod = nrxn.products


Looks like prod isn't used below

alongd · 2019-07-18T17:01:45Z

rmgpy/rmg/pdep.py

+        nrxns = []
+        for nrxn in self.netReactions:
+           prod = nrxn.products
+           if nrxn.products not in keptProducts or nrxn.reactants not in keptProducts:


nrxn.products is a list of length 1 to 3, right? Can an in or not in check be performed that way? Shouldn't we iterate through the products in nrxn.products before the check?

keptProducts is constructed as a list of lists of species (from a pdep network perspective this makes sense since each represents a configuration) so this should work fine.

alongd · 2019-07-18T17:05:05Z

rmgpy/rmg/pdep.py

+
+        con = np.linalg.cond(A)
+
+        try:


I think the error message and comment are a bit confusing: So if it is ill-conditioned we raise an error, but if it is "very ill-conditioned" we troubleshoot?

I've adjusted this to use an if statement rather than the first try except, which makes it a bit clearer. In general if we expect we can handle the matrix we do it normally, otherwise we fall back to arbitrary precision arithmetic, and lastly if we can't manage that we just assume all the isomers that aren't reactant channels have zero concentration which makes the calculation trivial.

alongd · 2019-07-18T17:05:55Z

rmgpy/rmg/pdep.py

        return filtered_rxns

-    def get_rate_filtered_reactions(self,T,P,tol):
+    def get_rate_filtered_products(self,T,P,tol):


Add space between comma and argument

alongd · 2019-07-18T17:09:29Z

arkane/explorer.py

+                    for prod in productSet:
+                        logging.info([x.label for x in prod])
+
+                if rxnSet:


maybe combine these conditions into the ones above?

alongd · 2019-07-18T17:11:43Z

rmgpy/rmg/pdep.py

        self.Nprod = len(self.products)

-    def remove_reactions(self,reactionModel,rxns):
+    def remove_reactions(self,reactionModel,rxns=None,prods=None):


add spaces?

alongd · 2019-07-18T17:12:19Z

rmgpy/rmg/pdep.py

-        for rxn in rxns:
-            self.pathReactions.remove(rxn)
-
+        if rxns:


shouldn't this be if rxns is not None? Or is there no practical difference?

There's no practical difference it won't trigger if rxns = None or rxns = [].

alongd · 2019-07-18T17:13:57Z

rmgpy/rmg/pdepTest.py

        self.pdepnetwork.explored = []

-
-    def test_SS_solver(self):


But pdep.py still had the method, doesn't it? (or was the method itself removed as well?)
If we're still using the original method somewhere, lets keep/relocate the test for it?

So the method exists, but the problem is that pdepTest doesn't have appropriate objects to test it directly after changes made in solve_SS_network. test_flux_filter does test it though, so it's still under coverage.
I invested sometime trying to figure out how to make it work, but given it's already under reasonable coverage (get_rate_filtered_products isn't a complicated function), it didn't seem worth it.

mjohnson541 · 2019-07-30T18:08:53Z

@alongd

alongd

OK, looks good. Just a minor comment

alongd · 2019-07-30T18:38:57Z

rmgpy/rmg/pdep.py


-        c = c.astype(np.float64)
-
+            try:


Maybe only leave the critical part (qr_solve?) inside the try: except block?

So the problem is that nnls (non-negative least squares) is critical as well when required and may fail as well (although given the qr_solve has to succeed to try nnls it shouldn't fail that often, but it should still fall back properly if nnls is called and fails).

alongd · 2019-08-02T02:46:41Z

OK, I think we're all set. Can you rebase?

… networks in Arkane

…ved properly

…recision also if arbitrary precision fails fall back to just using rate coefficients

this includes adding the fallback algorithm that simply uses the rate coefficients rather than the steady state analysis

…n terms of path reactions

note that the test_SS_solver is removed because we've switched from using pathReactions to netReactions

alongd

Thanks!

mjohnson541 self-assigned this Jul 9, 2019

mjohnson541 added Arkane Complexity: Medium labels Jul 9, 2019

mjohnson541 force-pushed the pdepnetworkgen3 branch 2 times, most recently from ff67fdf to 505333f Compare July 9, 2019 20:05

mjohnson541 requested review from alongd and removed request for alongd July 11, 2019 18:39

amarkpayne requested a review from lily90502 July 11, 2019 18:39

alongd self-requested a review July 11, 2019 18:40

mjohnson541 removed the request for review from alongd July 11, 2019 18:40

alongd reviewed Jul 18, 2019

View reviewed changes

mjohnson541 force-pushed the pdepnetworkgen3 branch 2 times, most recently from 82e7071 to 2b0730a Compare July 23, 2019 20:00

alongd reviewed Jul 30, 2019

View reviewed changes

alongd added the Before Py3 Should be merged before Python 3 transition label Aug 1, 2019

mjohnson541 added 8 commits August 2, 2019 12:06

check forbidden structures in edge when generating pressure dependent…

3773306

… networks in Arkane

adjust how we remove disconnected products so that everything is remo…

bfe277f

…ved properly

try a regular linear algebra solve before falling back to arbitrary p…

7fcf533

…recision also if arbitrary precision fails fall back to just using rate coefficients

adapt get_rate_filtered_reactions to get_rate_filtered_products

854bbe9

this includes adding the fallback algorithm that simply uses the rate coefficients rather than the steady state analysis

do flux reduction in terms of products of net reactions rather than i…

8ccf5c9

…n terms of path reactions

adapt network filtering tests

f3b3f02

note that the test_SS_solver is removed because we've switched from using pathReactions to netReactions

adapt explorerTest.py file to bugfixes and changes in flux reduction

8ce12f2

fix spacing in rmg/pdep.py

48f12c8

mjohnson541 force-pushed the pdepnetworkgen3 branch from 2b0730a to 48f12c8 Compare August 2, 2019 16:06

alongd approved these changes Aug 4, 2019

View reviewed changes

alongd merged commit d66fbfe into master Aug 4, 2019

alongd deleted the pdepnetworkgen3 branch August 4, 2019 02:07

mliu49 mentioned this pull request Nov 26, 2019

RMG v3.0.0 Release Planning #1830

Closed

mliu49 mentioned this pull request Dec 16, 2019

RMG-Py v3.0.0 Release #1852

Merged

Conversation

mjohnson541 commented Jul 9, 2019

Uh oh!

codecov bot commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alongd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjohnson541 Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjohnson541 commented Jul 30, 2019

Uh oh!

alongd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alongd commented Aug 2, 2019

Uh oh!

alongd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Jul 9, 2019 •

edited

Loading

mjohnson541 Jul 19, 2019 •

edited

Loading