SLACKHA / pyJac

Creates C and CUDA analytical Jacobians for chemical kinetics ODE systems
http://slackha.github.io/pyJac/
MIT License
52 stars 23 forks source link

Fix for #26 and other minor updates #27

Closed skyreflectedinmirrors closed 6 years ago

skyreflectedinmirrors commented 6 years ago

-correct temperature range column -check all species in reaction exist -fix for non-unity default-efficiency (should solve #26) -duplicate troe warning for chemically activated reactions

kyleniemeyer commented 6 years ago

I think this looks good—did you double check that it doesn't mess anything up that previously works?

skyreflectedinmirrors commented 6 years ago

@kyleniemeyer I'll run a test on GRI-3.0 real quick to make sure it didn't break anything

skyreflectedinmirrors commented 6 years ago

@michael-a-hansen is there an option to specify a default_efficiency in a .cti file? I can't seem to find it. I'm trying to figure out if this problem shows up in any mechanism we've tested (if not, a simple check of GRI-mech should tell me whether or not I broke anything)

michael-a-hansen commented 6 years ago

I can't find a default efficiency specification in any CTI files. It looks like the only case a default efficiency is zero is when the species is specified directly, as in H + O2 (+ AR) <=> HO2 (+ AR). It certainly looks like the default efficiency will always be 1 or 0, but I suppose it's good to either catch that and/or match cantera behavior anyway.

skyreflectedinmirrors commented 6 years ago

but I suppose it's good to either catch that and/or match cantera behavior anyway

Agreed! I don't think the fix here should change anything for 99.9% of mechanisms, but Cantera does allow (or at least, doesn't reject) setting the third-body efficiency of a single species third-body (as in the test case you gave).

skyreflectedinmirrors commented 6 years ago

Testing on the first GRI-mech PaSR database gave:

>>> np.mean(err['err_jac_thr']) / err['err_jac_thr'].size
2.0206057589142586e-07 %
>>> np.max(err['err_jac_thr_max'])
0.0067045055745845851 %

which is ~ the numbers in Table 3 of the paper (and the same as the previous commit before this PR), so i think we should be good here

skyreflectedinmirrors commented 6 years ago

@kyleniemeyer I've updated the changelog as well. This might be a good time to do a new release as well (bumping up to 1.0.5.b1). If we don't have any more immediate concerns, I think we should merge this in

kyleniemeyer commented 6 years ago

OK, seems good. Agreed on bumping the version number.