MNT: Cdflib update #991

tech3371 · 2024-10-08T16:01:35Z

Change Summary

closes #606

Overview

Updated cdflib to it's latest version. Made changes where I can but some was outside of what I can do. For those, I marked pytest to skip and need to address in future PR.

Updated Files

Instruments updated

CoDICE
- removed label axis because many of variable using the default has more than one dimension. As a result, it would error because ISTP doesn't allow to set both label axis and PTR label.
- skipped few tests because sample data was not increasing monotonically.
GLOWS
- added changes to include LABL_PTR_1
IMAP-Hi
- added changes to include LABL_PTR_1
MAG
- added changes to include LABL_PTR_1
SWE
- removed label axis because ISTP doesn't allow to set both label axis and PTR label.
imap_processing/cdf/config/shared/default_variable_cdf_attrs_schema.yaml
- set some of attrs's required to false because some those are only required for epoch variable and not other data type. Also, LABLAXIS is not required if 'LABL_PTR_iis set. These will be taken care of insammi`. Changes I made is temporary fixes.

Testing

I had to skip few tests due to data not increasing monotonically. I will create issue for those that needs update.

maxinelasp

Why doesn't LABLAXIS work here? Is that an error out of cdflib? Overall it looks good to me, I'm just wondering why LABLAXIS doesn't work...

tech3371 · 2024-10-08T17:32:31Z

Why doesn't LABLAXIS work here? Is that an error out of cdflib? Overall it looks good to me, I'm just wondering why LABLAXIS doesn't work...

@maxinelasp There is two parts to this. If schema defines a required field, it will add that and fill it with empty string even if we don't include it our own YAML file. This happens from this line. Since ISTP doesn't allow to set both LABLAXIS and LABL_PTR_i, cdflib will complain that we had set LABLAXIS and LABL_PTR_i even though user didn't but schema did.

tech3371 · 2024-10-08T17:37:37Z

imap_processing/tests/codice/test_codice_l1a.py

@@ -89,6 +89,7 @@ def test_l1a_data(request) -> xr.Dataset:
    return dataset


+@pytest.mark.xfail(reason="Epoch variable data needs to monotonically increase")


I created new issue to capture this #992

tech3371 · 2024-10-08T17:38:31Z

imap_processing/tests/codice/test_codice_l1a.py

@@ -167,6 +169,7 @@ def test_l1a_data_array_values(test_l1a_data: xr.Dataset, validation_data: Path)
            )


+@pytest.mark.xfail(reason="Epoch variable data needs to monotonically increase")


captured this work in #993

maxinelasp · 2024-10-08T17:44:33Z

But, if we mark both as required: false then do we need to update the yaml files? I thought that was the only change needed.

tech3371 · 2024-10-08T18:05:41Z

But, if we mark both as required: false then do we need to update the yaml files? I thought that was the only change needed.

Copy of Slack conversation in case someone is wondering same thing.

(Tenzin) If I understand what you are thinking, are you thinking that we set both false and then update our own attrs YAML to include LABLAXIS or LABL_PTR_i? If so, yes. That’s what I did in that PR with data variable who had more than one dimensions. cdflib requires to add LABL_PTR_i for data variable who had more than one dimension. Therefore, I had to make those additional changes to be ISTP compliant. For 2d or higher dimension data, we remove LABLAXIS and replace with LABL_PTR_i.

subagonsouth

Thanks for taking this on! I have a couple of nit picks for Hi changes. One thing that we should make consistent across all instruments is that label variables should be variables, not coordinates. It would make sense to put that in it's own PR.

For my Hi nits, I am happy to make a commit with the changes since they are purely stylistic things.

subagonsouth · 2024-10-08T21:58:59Z

imap_processing/hi/l1a/histogram.py

@@ -108,6 +108,13 @@ def allocate_histogram_dataset(num_packets: int) -> xr.Dataset:
        dims=["angle"],
        attrs=attr_mgr.get_variable_attributes("hi_hist_angle"),
    )
+    coords["angle_label"] = xr.DataArray(


Label variables should not be added to the coordinates list. They are just variables.

This LABL_PTR_i confused me a lot. Changes in this file and XML file is related to that. This is something SPDF and ISTP caused. When your data has great than 1D array, it requires to have label for each DEPEND_1 and onward. And we need to create xr.DataArray for those labels and need to add those to coordinate, otherwise cdflib will throw error.

FAILED imap_processing/tests/hi/test_l1a.py::test_app_hist_decom - cdflib.xarray.xarray_to_cdf.ISTPError: ISTP Compliance Warning: variable qual_ab listed angle_label as its LABL_PTR_1. However, it was not found in the dataset.

Suggested change

coords["angle_label"] = xr.DataArray(

data_vars["angle_label"] = xr.DataArray(

I think that is what @subagonsouth is suggesting. cdflib fails with that as well? Coordinates should be optional, so this might be an issue to raise with cdflib.

moving this to data variable. I will refactor other instrument's label in new PR. I do want to point out that other past mission did store their coordinate label in coordinate as well. I wonder if they did that to be able to select and filter data easier later on. That's something we should think about. I will check with Andriy first before I refactor other instrument.

imap_processing/cdf/config/imap_hi_variable_attrs.yaml

imap_processing/hi/l1a/histogram.py

subagonsouth

Thanks for doing this!

tech3371 · 2024-10-11T16:46:16Z

imap_processing/tests/hi/test_l1a.py

@@ -62,6 +63,9 @@ def test_app_nhk_decom(hi_l0_test_data_path):
    assert cem_raw_cdf_filepath.name == "imap_hi_l1a_45sensor-hk_20100313_v001.cdf"


+@pytest.mark.skip(


captured this work in #1009

tech3371 added 6 commits October 7, 2024 16:09

update cdflib

b32c417

MAG cdflib updates

e56286b

CoDICE cdflib updates

112aa7e

GLOWS cdflib updates

9fefce1

SWE cdflib updates

c66bdf3

IMAP-Hi cdflib updates

d574419

tech3371 added bug Something isn't working enhancement New feature or request CDF Related to CDF files labels Oct 8, 2024

tech3371 self-assigned this Oct 8, 2024

tech3371 requested review from bourque, subagonsouth and maxinelasp October 8, 2024 16:01

maxinelasp approved these changes Oct 8, 2024

View reviewed changes

This was referenced Oct 8, 2024

Update CoDICE pytest with new data #992

Closed

Update MAG pytest with new data #993

Open

tech3371 commented Oct 8, 2024

View reviewed changes

subagonsouth reviewed Oct 8, 2024

View reviewed changes

feedback changes

748b254

tech3371 changed the title ~~Cdflib update~~ MNT: Cdflib update Oct 10, 2024

subagonsouth approved these changes Oct 10, 2024

View reviewed changes

feedback changes and skip and add note about test

25d8bf2

tech3371 mentioned this pull request Oct 11, 2024

Update Hi Histogram test with new data #1009

Closed

tech3371 commented Oct 11, 2024

View reviewed changes

tech3371 merged commit 01d3464 into IMAP-Science-Operations-Center:dev Oct 11, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNT: Cdflib update #991

MNT: Cdflib update #991

tech3371 commented Oct 8, 2024

maxinelasp left a comment

tech3371 commented Oct 8, 2024

tech3371 Oct 8, 2024

tech3371 Oct 8, 2024

maxinelasp commented Oct 8, 2024

tech3371 commented Oct 8, 2024 •

edited

Loading

subagonsouth left a comment

subagonsouth Oct 8, 2024

tech3371 Oct 9, 2024

greglucas Oct 10, 2024

tech3371 Oct 11, 2024 •

edited

Loading

subagonsouth left a comment

tech3371 Oct 11, 2024

		@@ -89,6 +89,7 @@ def test_l1a_data(request) -> xr.Dataset:
		return dataset


		@pytest.mark.xfail(reason="Epoch variable data needs to monotonically increase")

		@@ -167,6 +169,7 @@ def test_l1a_data_array_values(test_l1a_data: xr.Dataset, validation_data: Path)
		)


		@pytest.mark.xfail(reason="Epoch variable data needs to monotonically increase")

	coords["angle_label"] = xr.DataArray(
	data_vars["angle_label"] = xr.DataArray(

		@@ -62,6 +63,9 @@ def test_app_nhk_decom(hi_l0_test_data_path):
		assert cem_raw_cdf_filepath.name == "imap_hi_l1a_45sensor-hk_20100313_v001.cdf"


		@pytest.mark.skip(

MNT: Cdflib update #991

MNT: Cdflib update #991

Conversation

tech3371 commented Oct 8, 2024

Change Summary

Overview

Updated Files

Testing

maxinelasp left a comment

Choose a reason for hiding this comment

tech3371 commented Oct 8, 2024

tech3371 Oct 8, 2024

Choose a reason for hiding this comment

tech3371 Oct 8, 2024

Choose a reason for hiding this comment

maxinelasp commented Oct 8, 2024

tech3371 commented Oct 8, 2024 • edited Loading

subagonsouth left a comment

Choose a reason for hiding this comment

subagonsouth Oct 8, 2024

Choose a reason for hiding this comment

tech3371 Oct 9, 2024

Choose a reason for hiding this comment

greglucas Oct 10, 2024

Choose a reason for hiding this comment

tech3371 Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

subagonsouth left a comment

Choose a reason for hiding this comment

tech3371 Oct 11, 2024

Choose a reason for hiding this comment

tech3371 commented Oct 8, 2024 •

edited

Loading

tech3371 Oct 11, 2024 •

edited

Loading