Repository navigation
Adding NAN support for positive temperature and density rules - #30
fleuryl-ai wants to merge 7 commits into
Conversation
Added import for numpy to generic.py for validation.
Co-authored-by: Prasad <prasad.sawantdesai@gmail.com>
Co-authored-by: Prasad <prasad.sawantdesai@gmail.com>
|
Let's converge first on #29 test, I see no reason for the rules testing density and temperature positiveness to be implemented differently, or is there one? |
|
Probably, since #29 supports all NaNs in the data, given that I thought the opposite yesterday. |
| """Validate that temperature and energy values are positive.""" | ||
| for node in Select(ids, r"^((?!_error_).)*$", has_value=True): | ||
| if node.metadata.units == "eV" and "derivative" not in str(node.metadata.path): | ||
| assert np.all(np.logical_or(np.isnan(node), node >= 0)), ( |
There was a problem hiding this comment.
We don't need np.all, confirm @prasad-sawantdesai ?
| "derivative" in str(node.metadata.path) | ||
| """Validate that density values are positive.""" | ||
| for node in Select(ids, r"^((?!_error_).)*$", has_value=True): | ||
| if getattr(node.metadata, "units", None) == "m^-3" and "derivative" not in str( |
There was a problem hiding this comment.
Do we need using getattr here?
| # If _max exists and is filled, check that it is >= _min | ||
| if quantity_max is not None and quantity_max.has_value: | ||
| assert quantity_min <= quantity_max | ||
| def _extract_clean_array(node): |
There was a problem hiding this comment.
Do we really need this helper function here?
I think that changing the assert line to np.nan_to_num(quantity_min, nan=-np.inf) <= np.nan_to_num(quantity_max, nan=np.inf) shall be enough
|
Are we speaking of explicit NaN values or of the IMAS_INVALID value here (-9e40) ? |
This was NaN values (not assuming about the reasons they got written in the data by the codes, this may be very tricky to separate use-cases of NaNs for numerical reasons vs for non-IMAS-standard marking absence of the value) If use-cases demand a rule to detect NaNs existence this shall be a different rule (optional one, meaning not in the generic bundle, as the IMAS DD does not rule out NaNs or am I wrong?). |
Ok, good idea ... it will simplify our lives to allow keeping NaNs in the signals. And if someone stricter wants to detect them as well, he can indeed implement an optional rule. NaN are not ruled out by the IMAS DD, but historically we defined IMAS_INVALID instead because NaN were not recognized by all languages / compilers (as far as I can remember). |
|
@fleuryl-ai I have added changes in my branch here. Could you please check if it works for you? If you are agree, we can update these changes here.. |
This branch has not been deployed
4 participants
This PR updates validate_density_positive and validate_temperature_positive to correctly handle datasets containing NaN values, preventing False Positives in presence of NaN values in the data.
Changes Made
NaN filtering: apply boolean masking (arr[~np.isnan(arr)]) before running np.min() checks.
Test Coverage
Both patched validators have been thoroughly tested and verified across the following 4 scenarios:
Invalid data (negative values): correctly raises an AssertionError when strictly negative values are present.
Valid data (all positive values): validation passes as expected.
Valid data with mixed NaNs: correctly ignores NaN entries and validates that all remaining non-NaN numeric values are non-negative.
Valid data with only NaNs: successfully handles 100% NaN datasets as valid without raising false positive assertions.