Improve Rule Generation #2277
Improve Rule Generation #2277
Conversation
…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
|
This pull request introduces 2 alerts and fixes 2 when merging ea72bf4 into 603b7f4 - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 2 alerts and fixes 2 when merging 4174c91 into 603b7f4 - view on LGTM.com new alerts:
fixed alerts:
|
| kr.fit_to_data(Tlist, klist, reverse_units, kf.T0.value_si) | ||
| return kr | ||
|
|
||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Can you resolve this? It seems some version conflicts are committed
| kr.fit_to_data(Tlist, klist, reverse_units, kf.T0.value_si) | ||
| return kr | ||
|
|
||
| >>>>>>> 3afc9cc91 (handle ChargeTransfer kinetics with in reaction.py) |
| kunits = get_rate_coefficient_units_from_reaction_order(n_gas, n_surf) | ||
|
|
||
| kf = self.kinetics | ||
| <<<<<<< HEAD |
| return self.reverse_arrhenius_charge_transfer_rate(kf, kunits, Tmin, Tmax) | ||
|
|
||
| elif isinstance(kf, KineticsData): | ||
| >>>>>>> 3afc9cc91 (handle ChargeTransfer kinetics with in reaction.py) |
There was a problem hiding this comment.
Could you remove this and resolve the conflicts?
| elif self.electrons > 0: | ||
| products_net_charge -= self.electrons | ||
|
|
||
| >>>>>>> 3afc9cc91 (handle ChargeTransfer kinetics with in reaction.py) |
There was a problem hiding this comment.
Could you remove this and resolve the conflicts?
| kinetics = kinetics.to_arrhenius(rxn.get_enthalpy_of_reaction(T)) | ||
| L = list(set(template_rxn_map[entry.label]) - set(rxns_test)) | ||
|
|
||
| if L != []: |
There was a problem hiding this comment.
I think a more Pythonic way to check empty list is by if not L?
hwpang
left a comment
hwpang
left a comment
There was a problem hiding this comment.
@mjohnson541 Thanks for the PR! I have a few comments. Could you rebase as well?
| 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) |
There was a problem hiding this comment.
I think this is unreachable?
if n==1: # on line 4611
else: # line 4631
if n==1: # line 432
| if entry.parent: | ||
| entry = entry.parent | ||
|
|
||
| boo = True |
There was a problem hiding this comment.
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: ?
| return out | ||
|
|
||
| if abs(dHrxn) > 4 * w0 / 10.0: | ||
| E0 = w0 / 10.0 |
There was a problem hiding this comment.
can you explain why removing this? (in the commit message, ideally)
|
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? |
|
I think this would address #1748 |
|
I think this is now part of #2316 which has been more recently rebased and checked. Hopefully that will be merged soon. |
This has four main changes