Improve Rule Generation #2277

mjohnson541 · 2022-02-28T22:09:55Z

This has four main changes

Enables tree generation for charge transfer families
Updates tree generation, if E0 < 0 or n = 1 it simply uses the average instead of the BM rule
Improves BM fitting using dH298
Adds a SolventData attribute to reactions that can hold transition state solute information

…ansfer for rms output

1) Enable fitting of ArrheniusChargeTransferBM 2) just average the kinetics if n=1 or E0<0

ascend based on original uncertainties and compute errors at that node in cross validate

consistenty use dH298 do not estimate e0 = w0/10 when |dHrxn| > 0.4w0

lgtm-com · 2022-02-28T22:36:34Z

This pull request introduces 2 alerts and fixes 2 when merging ea72bf4 into 603b7f4 - view on LGTM.com

new alerts:

1 for Syntax error
1 for Unreachable code

fixed alerts:

2 for Suspicious unused loop iteration variable

lgtm-com · 2022-03-01T00:15:39Z

This pull request introduces 2 alerts and fixes 2 when merging 4174c91 into 603b7f4 - view on LGTM.com

new alerts:

1 for Syntax error
1 for Unreachable code

fixed alerts:

2 for Suspicious unused loop iteration variable

hwpang · 2022-10-06T21:14:22Z

rmgpy/reaction.py

@@ -880,9 +877,60 @@ def reverse_sticking_coeff_rate(self, k_forward, reverse_units, surface_site_den
        kr.fit_to_data(Tlist, klist, reverse_units, kf.T0.value_si)
        return kr

+<<<<<<< HEAD


Can you resolve this? It seems some version conflicts are committed

hwpang · 2022-10-06T21:15:01Z

rmgpy/reaction.py

+        kr.fit_to_data(Tlist, klist, reverse_units, kf.T0.value_si)
+        return kr
+
+>>>>>>> 3afc9cc91 (handle ChargeTransfer kinetics with in reaction.py)


Could you remove this

hwpang · 2022-10-06T21:15:16Z

rmgpy/reaction.py

                            "Cannot determine phases of species. We will assume gas"
                            )
        n_surf = len(surf_prods)
        n_gas = len(self.products) - len(surf_prods)
        kunits = get_rate_coefficient_units_from_reaction_order(n_gas, n_surf)

        kf = self.kinetics
+<<<<<<< HEAD


Could you remove this

hwpang · 2022-10-06T21:15:26Z

rmgpy/reaction.py

+            return self.reverse_arrhenius_charge_transfer_rate(kf, kunits, Tmin, Tmax)
+
+        elif isinstance(kf, KineticsData):
+>>>>>>> 3afc9cc91 (handle ChargeTransfer kinetics with in reaction.py)


Could you remove this and resolve the conflicts?

hwpang · 2022-10-06T21:15:44Z

rmgpy/reaction.py

+        elif self.electrons > 0:
+            products_net_charge -= self.electrons
+
+>>>>>>> 3afc9cc91 (handle ChargeTransfer kinetics with in reaction.py)


Could you remove this and resolve the conflicts?

hwpang · 2022-10-06T21:18:13Z

rmgpy/data/kinetics/family.py

-                            kinetics = kinetics.to_arrhenius(rxn.get_enthalpy_of_reaction(T))
+                L = list(set(template_rxn_map[entry.label]) - set(rxns_test))
+
+                if L != []:


I think a more Pythonic way to check empty list is by if not L?

hwpang

@mjohnson541 Thanks for the PR! I have a few comments. Could you rebase as well?

rwest

Need to figure out how this relates to #2264
I think both are trying to solve some similar problems.

Can you explain why NOT to fit a Blowers Masel when there's only one training reaction? (if that's the new proposal) ?

rwest · 2023-06-07T04:27:51Z

rmgpy/data/kinetics/family.py

+                return kin
+        else:
+            if n == 1:
+                kin.uncertainty = RateUncertainty(mu=0.0, var=(np.log(fmax) / 2.0) ** 2, N=1, Tref=Tref, data_mean=data_mean, correlation=label)


I think this is unreachable?

if n==1: # on line 4611 else: # line 4631 if n==1: # line 432

rwest · 2023-06-07T04:32:18Z

rmgpy/data/kinetics/family.py

@@ -3704,47 +3704,43 @@ def cross_validate(self, folds=5, template_rxn_map=None, test_rxn_inds=None, T=1
                    if entry.parent:
                        entry = entry.parent

+                boo = True


Why boo? A more informative name would help. ANd I don't see where boo is ever set to anything but True? So what's this infinite loop about?
Does it mean while entry.parent: ?

rwest · 2023-06-07T04:41:14Z

rmgpy/kinetics/arrhenius.pyx

            def kfcn(E0):
                Vp = 2 * w0 * (2 * w0 + 2 * E0) / (2 * w0 - 2 * E0)
                out = Ea - (w0 + dHrxn / 2.0) * (Vp - 2 * w0 + dHrxn) * (Vp - 2 * w0 + dHrxn) / (Vp * Vp - (2 * w0) * (2 * w0) + dHrxn * dHrxn)
                return out

-            if abs(dHrxn) > 4 * w0 / 10.0:
-                E0 = w0 / 10.0


can you explain why removing this? (in the commit message, ideally)

mjohnson541 · 2023-06-07T05:01:40Z

I'm not sure how up to date this PR is relative to the electrochem branch. It's been a while, I think as defined in the paper the dHrxn is at 298 K which implies that the Ea is constant with respect to temperature so you only have one datapoint to determine E0. You can determine E0 this way, but I think someone was having problems with those fits? At some point I think this was a matter of significant urgency to get merged and then we (or at least I) forgot about it?

rwest · 2023-06-22T18:59:07Z

I think this would address #1748

rwest · 2023-07-18T19:08:35Z

I think this is now part of #2316 which has been more recently rebased and checked. Hopefully that will be merged soon.

mjohnson541 added 15 commits February 28, 2022 16:40

add solute data to KineticsModel and Arrhenius

e6110e9

add ArrheniusChargeTransfer kinetics type

a51925e

create ArrheniusChargeTransferBM type

aa39337

add ChargeTransfer types to kinetics/__init__.py

95db806

handle ChargeTransfer kinetics with in reaction.py

25807c1

add handling ArrheniusChargeTransfer and fix handling SurfaceChargeTr…

908bad9

…ansfer for rms output

enable access to SoluteData object in kinetics database

68d43d5

when rule making is done on one processor don't use pool

8d30987

add function for averaging kinetics

d81d824

update rule fitting

1898402

1) Enable fitting of ArrheniusChargeTransferBM 2) just average the kinetics if n=1 or E0<0

enable use of non-surface charge transfer families

5c727f1

add charge transfer types to database context

321dbb8

standardize ascend option in cross validate

1601dc8

ascend based on original uncertainties and compute errors at that node in cross validate

add comment to averaged kinetics

58a9ef0

fix BM fitting

ea72bf4

consistenty use dH298 do not estimate e0 = w0/10 when |dHrxn| > 0.4w0

added Faraday's Constant F

4174c91

hwpang reviewed Oct 6, 2022

View reviewed changes

hwpang requested changes Oct 6, 2022

View reviewed changes

rwest mentioned this pull request Jun 7, 2023

ArrheniusBM fitting procedure #2264

Closed

rwest reviewed Jun 7, 2023

View reviewed changes

rwest closed this Jul 18, 2023

rwest linked an issue Jul 20, 2023 that may be closed by this pull request

Blowers Masel Arrhenius trained and evaluated with T-varying ∆Hr(T) ? #1748

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Rule Generation #2277

Improve Rule Generation #2277

mjohnson541 commented Feb 28, 2022

lgtm-com bot commented Feb 28, 2022

lgtm-com bot commented Mar 1, 2022

hwpang Oct 6, 2022 •

edited

hwpang Oct 6, 2022

hwpang Oct 6, 2022

hwpang Oct 6, 2022

hwpang Oct 6, 2022

hwpang Oct 6, 2022

hwpang left a comment

rwest left a comment

rwest Jun 7, 2023

rwest Jun 7, 2023

rwest Jun 7, 2023

mjohnson541 commented Jun 7, 2023

rwest commented Jun 22, 2023

rwest commented Jul 18, 2023

Improve Rule Generation #2277

Improve Rule Generation #2277

Conversation

mjohnson541 commented Feb 28, 2022

lgtm-com bot commented Feb 28, 2022

lgtm-com bot commented Mar 1, 2022

hwpang Oct 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwpang left a comment

Choose a reason for hiding this comment

rwest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjohnson541 commented Jun 7, 2023

rwest commented Jun 22, 2023

rwest commented Jul 18, 2023

hwpang Oct 6, 2022 •

edited