Save errors in create_field #167
Conversation
jl-wynen
force-pushed
the
save-variances
branch
from
9ca8837 to
c2ae457
Compare
| values_dataset.attrs.update(attrs) | ||
| if errors is not None: | ||
| errors_dataset = group.create_dataset(name + '_errors', data=errors, **kwargs) | ||
| errors_dataset.attrs.update(attrs) |
There was a problem hiding this comment.
I am not sure the units attribute (or any others) should we written here?
There was a problem hiding this comment.
On the other hand... I can't see any harm, even if the standard does not require it?
There was a problem hiding this comment.
I think the standard is silent on this. I did it just in case they are useful. But I'm fine with not writing them.
There was a problem hiding this comment.
Can you check with Tobias? If the standard does not specify them, maybe we should not write them either. Otherwise we may end up in situations where our reading code (which uses this for testing) only works with files that where written using ScippNexus.
jl-wynen
force-pushed
the
save-variances
branch
from
76c9035 to
5230d5b
Compare
src/scippnexus/base.py
Outdated
| @@ -235,13 +235,14 @@ def _make_child(obj: Union[H5Dataset, H5Group]) -> Union[Field, Group]: | |||
| items = {k: v for k, v in items.items() if not k.startswith('cue_')} | |||
| for suffix in ('_errors', '_error'): | |||
| field_with_errors = [name for name in items if f'{name}{suffix}' in items] | |||
| print(suffix, field_with_errors) | |||
There was a problem hiding this comment.
| print(suffix, field_with_errors) |
There was a problem hiding this comment.
Oops. We should use https://pypi.org/project/flake8-print/. Or switch to Ruff like Scitacean: https://docs.astral.sh/ruff/rules/#flake8-print-t20
jl-wynen
force-pushed
the
save-variances
branch
from
9eafdaa to
55d6ff3
Compare
| @@ -434,6 +434,24 @@ def test_errors_read_as_variances(h5root): | |||
| assert np.array_equal(dg['time'].variances, np.arange(5.0) ** 2) | |||
|
|
|||
|
|
|||
| def test_does_not_require_unit_of_errors(h5root): | |||
There was a problem hiding this comment.
With the latest change, is there a test that checks behavior for mismatching error units?
There was a problem hiding this comment.
Yes:
scippnexus/tests/nexus_test.py
Line 215 in 54642fe
Fixes #105