Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exporting to Java syntax messes up uncertainty units #21

Closed
rwest opened this issue Aug 29, 2013 · 1 comment
Closed

Exporting to Java syntax messes up uncertainty units #21

rwest opened this issue Aug 29, 2013 · 1 comment

Comments

@rwest
Copy link
Member

rwest commented Aug 29, 2013

Import this:

533.    C_methane   C2b                 294-376     7.5e12  0.0  0  1.05       1.6e12  0.0 0   0.12 4   Matsugi et al 10.1021/jp1012494

and you get this

    kinetics = ArrheniusEP(
        A = (7500000000000.0, 'cm^3/(mol*s)', '+|-', 1600000000000.0),
        n = 0,
        alpha = 0,
        E0 = (1.05, 'kcal/mol', '+|-', 0.12),
        Tmin = (294, 'K'),
        Tmax = (376, 'K'),
    ),

which becomes this

533   C_methane    C2b   294-376  7.50e+12  0.00  0.00  1.05 1.6e+18  0  0 2.86807e-05   4   Matsugi et al 10.1021/jp1012494

when you export it again.
The uncertainty on A is six orders of magnitude larger (m3/cm3), and on Ea is 4184 times smaller (J/kCal).

@rwest
Copy link
Member Author

rwest commented Aug 29, 2013

This particular issue is fixed by ReactionMechanismGenerator/RMG-Py@fed3b59 but I have opened ReactionMechanismGenerator/RMG-Py#144 to remind us to look for other occurrences of uncertainty units being wrong.

@rwest rwest closed this as completed Aug 29, 2013
pierrelb pushed a commit to pierrelb/RMG-Py that referenced this issue Nov 1, 2013
In commit 851b2ba it was changed so that
Quantitiy.value and Quantity.uncertainty were now in customizable units,
and Quantitiy.value_si was the SI value. However, there was no uncertainty_si.

Now the uncertainty is also stored in SI units, like the value, and converted
to output units on demand. This change should be transparent, i.e. not change 
anything that was correct before, but it does create a new Quantity.uncertainty_si 
property, which is what we should have been using instead of .uncertainty in 
various places. i.e. we now need to go around using this to fix other bugs,
such as @GreenGroup/RMG-databaseReactionMechanismGenerator#21 ReactionMechanismGenerator/RMG-database#21

Also to do: do the same thing for the ArrayQuantity class.
pierrelb pushed a commit to pierrelb/RMG-Py that referenced this issue Nov 1, 2013
…tabase rules.

This should close @GreenGroup/RMG-databaseReactionMechanismGenerator#21 ReactionMechanismGenerator/RMG-database#21
But there are probably other places the same bug exists (i.e. foo.uncertainty should be replaced
with foo.uncertainty_si)

Also, why don't any of the Quantity unit tests check uncertainties at all? Hmm...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant