Mag L1D Repair attributes #2441

maxinelasp · 2025-11-18T00:36:19Z

Change Summary

Fixes #2297 along with another bug introduced in #2378. Also completes the attributes for L1D.

Overview

Add new attributes for L1D - which are very similar to the ones for L2
Fix the ancillary files so they do not upload through the write_cdf and upload flow
Fix the ancillary files so errors do not interrupt science processing upload

Testing

Added a test for the bugs that I saw causing this issue

imap_processing/cdf/config/imap_mag_l2_variable_attrs.yaml

greglucas · 2025-11-18T15:55:44Z

imap_processing/cli.py

+                version = self.version
+
+                output_filename = (
+                    imap_data_access.AncillaryFilePath.generate_from_inputs(


Rather than recreating this through the subclass are you able to put this logic into the write_cdf() function instead? It seems like maybe the only difference is in the filenaming convention here, so you could put a cut-out for that in the write_cdf() I think where you check for mag l1d in there. This just reads as a lot of duplicate code and potential for confusion by calling super in a few different paths here.

That was my initial thought, but it would require skipping a lot of write_cdf, and most of this code would still exist because it actually is pretty different than what write_cdf does. write_cdf is reading these values from the attributes of the dataset rather than from the class definition, so write_cdf doesn't even have access to the values I use here. So, moving this into write_cdf would require assigning these attributes into the dataset anyway (which is about as much code) and then adding another special case into write_cdf which skips most of it. I didn't want to clutter up shared code with the special case when it's not needed.

You have a good point about calling super(), I can move all the calls to the bottom so we have this cut out which pulls out the ancillary files and writes them, but then in most cases the call just routes through the parent classes function like normal.

The only code that actually matches with write_cdf is the split() call on the Logical_source and the xarray_to_cdf call at the end (where I pass in different kwargs.)

So, moving this into write_cdf would require assigning these attributes into the dataset anyway (which is about as much code) and then adding another special case into write_cdf which skips most of it. I didn't want to clutter up shared code with the special case when it's not needed.

This sounds like it might be nice anyways to add those attributes? I agree though, if it isn't a one/two-line cut-out for ancillary files then it probably doesn't make sense to put it into the write_cdf() function. I thought it just looked like it would only be this one naming line because they are cdf files as well.

greglucas · 2025-11-18T15:56:41Z

imap_processing/cli.py

+        try:
+            self.upload_products(ancillary_files)
+        except Exception as e:
+            logger.warning(f"Failed to upload ancillary products due to error {e}")


If these are output products don't we think they should be done with the same care as science products? I'm not sure why these would fail when a science product wouldn't.

Well, they are currently failing and blocking science product release. Since these are internal watchdog products, I do think they are a lower priority than the science files... but I can remove the excepts if that doesn't line up with the rest of the SDC or what MAG wants.

I'm just curious if there is a reason they are failing. Can we identify this earlier in the process somehow and not include these in the list of data products to upload if there is bad content. i.e. the upload isn't what should fail and if it does we should fix that at the source.

That is what the test I added is checking! But, partly the problem is that they are trying to get uploaded as science files and they aren't. So, I don't anticipate this happening again, but if it does, I was thinking it would be preferable to NOT block science products. However, this does reduce visibility into these products if they start failing so I understand your concern.
Really, these checks aren't for bad content, they're for A. is the filename constructed properly and B. is the ancillary file upload functioning properly.

But I am definitely open to removing them if the increased stability isn't worth the risk of missing errors from the SDC perspective.

In the post_processing() method there is a callout for Path vs Dataset difference in the list of products. So my personal preference would be to write the ancillary products here as you have (logging/ignoring errors if there are any if you want in that portion), then call super().post_process() with the mixed list of all science datasets and ancillary filepaths, and let the uploads happen there.

Oh yeah, that's a good catch that makes this cleaner as well. I'll make that change.

Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>

greglucas

minor nit on preferring a mixed list[Dataset, Path] to try and re-use the upload capability, but not blocking on that and looks good to me otherwise so you can take/leave that comment as you see fit.

greglucas · 2025-11-18T21:11:28Z

imap_processing/cli.py

+        try:
+            self.upload_products(ancillary_files)
+        except Exception as e:
+            logger.warning(f"Failed to upload ancillary products due to error {e}")


In the post_processing() method there is a callout for Path vs Dataset difference in the list of products. So my personal preference would be to write the ancillary products here as you have (logging/ignoring errors if there are any if you want in that portion), then call super().post_process() with the mixed list of all science datasets and ancillary filepaths, and let the uploads happen there.

alastairtree

Thanks Maxine! Added a couple of small inline bits and looking forward to a working L1D!

alastairtree · 2025-11-19T13:56:19Z

imap_processing/cdf/config/imap_mag_l2_variable_attrs.yaml

+
+vector_attrs_rtn:
+    <<: *vectors_default
+    CATDESC: Magnetic field vectors with x y z varying by time in Radial-Tangential-Normal Reference Frame


@mhairifin can you confirm if these are correct for me? Would not label these as r t and n not xyz?

That's correct, RTN specifically should be labelled as r, t, and n

alastairtree · 2025-11-19T14:03:01Z

imap_processing/mag/l1d/mag_l1d_data.py

        self,
        attribute_manager: ImapCdfAttributes,
        day: np.datetime64,
+        data_level: str = "l1d",


Should this be a field on MagL2L1dBase that is overridden in each subclass - I cant see why you want to be able to do MagL1d(..).generate_dataset(attributes, day, "some-other-level")?

alastairtree · 2025-11-19T14:04:38Z

imap_processing/mag/l1d/mag_l1d_data.py

-            # Call parent generate_dataset method
-            dataset = super().generate_dataset(attribute_manager, day)
+            # Call parent generate_dataset method with L1D data level
+            dataset = super().generate_dataset(attribute_manager, day, data_level="l1d")


Does not need the default value if the default is set by the method definition? Or as I said above move it to a field/property.

alastairtree · 2025-11-19T14:16:25Z

imap_processing/tests/mag/test_mag_l1d.py

+    mock_dependencies = ProcessingInputCollection()
+
+    with patch("imap_processing.cdf.utils.xarray_to_cdf"):
+        mag_processor.post_processing(l1d_datasets, mock_dependencies)


Any value in tests for the the burst files? Parameterised test? Similar for L2.

alastairtree · 2025-11-19T14:16:35Z

imap_processing/cli.py

+                        )
+                        # update the dataset in processed_data to point to a path
+                        processed_data[index] = output_filepath
+                    except Exception as e:


Can you catch anything more specific? I don't think you want to catch and continue on a MemoryError for example

maxinelasp added 3 commits November 13, 2025 12:19

Add attributes for L1D

b52f9f3

fix test

1f83c22

Update ancillary file uploads

2714255

maxinelasp requested review from a team, alastairtree, greglucas and mfacchinelli November 18, 2025 00:36

maxinelasp self-assigned this Nov 18, 2025

maxinelasp requested review from lacoak21 and subagonsouth and removed request for a team November 18, 2025 00:36

maxinelasp added the Ins: MAG Related to the MAG instrument label Nov 18, 2025

maxinelasp added this to IMAP Nov 18, 2025

greglucas reviewed Nov 18, 2025

View reviewed changes

maxinelasp and others added 2 commits November 18, 2025 09:33

Update imap_processing/cdf/config/imap_mag_l2_variable_attrs.yaml

487977d

Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>

rework post_processing() for clarity

1833376

greglucas approved these changes Nov 18, 2025

View reviewed changes

Simplify function, improve test

d80b0cb

alastairtree reviewed Nov 19, 2025

View reviewed changes

Mag L1D Repair attributes #2441

Are you sure you want to change the base?

Mag L1D Repair attributes #2441

Uh oh!

Conversation

maxinelasp commented Nov 18, 2025

Change Summary

Overview

Testing

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxinelasp Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greglucas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alastairtree left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maxinelasp Nov 18, 2025 •

edited

Loading