Update O2permeation.json #307
Conversation
Fixing the oxygen permeation data after a calculation error was found. Also added in uncertainty values.
Updated the formatting of the JSON file
Found a dataset that wasn't updated.
Fixed the pytests associated with the oxygen parameter JSON file changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #307 +/- ##
===============================================
+ Coverage 71.66% 74.67% +3.01%
===============================================
Files 40 41 +1
Lines 4524 4813 +289
===============================================
+ Hits 3242 3594 +352
+ Misses 1282 1219 -63 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pvdeg/data/O2permeation.json
Outdated
| { | ||
| "OX000": { | ||
| "comment": "This data is for oxygen permeation parameters. The activation energies are in [kJ/mol]. Do is in [cm²/s]. So is in [g/cm³/atm]. Po is in[g·mm/m²/day/atm]." | ||
| "comment": "This data is for oxygen permeation parameters. The activation energies are in [kJ/mol]. Do is in [cm\u00c2\u00b2/s]. So is in [g/cm\u00c2\u00b3/atm]. Po is in[g\u00c2\u00b7mm/m\u00c2\u00b2/day/atm]." |
There was a problem hiding this comment.
what is the reason for switching the units to this format for certain entries?
There was a problem hiding this comment.
I didn't do it, and I assumed that this was an automatic formatting thing. I prefer superscripts to be visualized properly and for the JSON to be more condensed with fewer carriage returns.
There was a problem hiding this comment.
I also prefer the actual superscript units, it's more readable and succinct. Let's go ahead and change these back to the "normal" readable units
There was a problem hiding this comment.
If it doesn't create future compatibility problems, I'll put it back to real superscripts.
There was a problem hiding this comment.
I think it should e fine, unless some of the tests failed because of this formatting(?) I'm not sure if these changes were part of your fix for some of the tests. If there are nbval or pytests that check print statements, then these might fail but these can easily be address by rerunning the notebooks for example.
Let's try and see what happens
pvdeg/data/O2permeation.json
Outdated
| "Ead": { | ||
| "name": "Diffusivity Activation Energy", | ||
| "units": "kJ/mol", | ||
| "value": 30.4829806851499, |
There was a problem hiding this comment.
should we really list values to this many significant figures? is this the true accuracy to which these values are known?
There was a problem hiding this comment.
No, we don't need so many. That is just me being lazy figuring very few people would actually look at it. Usually 2 sig figs is real, 3 eliminates relevant rounding error, and 4 is as precise as is reasonable. Should I go through and fix it? I'd suggest 4 sig figs unless otherwise justified.
There was a problem hiding this comment.
Yes, I think it would be a good idea to update these values. It's more technically accurate (even if very few people look at the file) and also just much cleaner
Could you elaborate in this issue what the calculation error was? It would be a useful reference for anyone who used to the old database/values |
For that set of data, it is a linear regression analysis using excel and an Arrhenius plot. |
Fixed formatting to use superscripts and special characters. and reduced the number of significant figures.
Fixed after changes to the O2 permation JSON file.
This is an update to the oxygen permeation values to fix a calculation error and to include uncertainty calculation results.