Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MNT: Update file naming convention via imap-data-access upgrade #363

Merged

Conversation

greglucas
Copy link
Collaborator

Change Summary

Overview

Bumps imap-data-access to v0.5.0 where there is a new filename requirement on the mission.

This is a major update to the filename convention, so there are lots of areas touched in this PR. Most are just updating the filenaming, but there is one other major update to the write_cdf() routine.

write_cdf() was changed to get all of its filename information from the attributes of the passed-in dataset directly, and uses ScienceFilePath from imap-data-access. There is an optional directory argument if you do want to write that dataset somewhere else, but I think this makes it a bit cleaner without requiring every instrument to create their filenames, but instead focus on the metadata, and then you get the filename for free.

New Dependencies

imap-data-access>=0.5

@greglucas greglucas requested review from a team, bourque, sdhoyt, tech3371, laspsandoval and maxinelasp and removed request for a team March 13, 2024 22:31
@greglucas
Copy link
Collaborator Author

Also note that I have just set all descriptors to "sci" for now. I'm not sure whether we are setting those in the CDF somewhere, or if we want to just set the CDF metadata attributes with that information directly. This is because the Logical_source previously did not contain the descriptor field, but now we are told it should. So it somewhat changes up the structure of attribute inheritance for all of the instruments and I wasn't sure how we wanted to do that, so figured it was easiest to leave for each instrument to follow-up individually themselves after this goes in to correct it.

@greglucas greglucas force-pushed the filename-update branch 2 times, most recently from ee0783c to 048b29a Compare March 15, 2024 17:12
Bumps imap-data-access to v0.5.0 where there is a new filename
requirement on the mission.
@greglucas
Copy link
Collaborator Author

Some notes/thoughts:

Copy link
Contributor

@laspsandoval laspsandoval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Contributor

@tech3371 tech3371 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple minor comments. Otherwise, it looks good to me!

docs/source/code-documentation/tools/xarray-to-cdf.rst Outdated Show resolved Hide resolved
@@ -154,7 +150,7 @@ def _validate_args(args):
if args.instrument not in imap_data_access.VALID_INSTRUMENTS:
raise ValueError(
f"{args.instrument} is not in the supported instrument list: "
f"{imap_processing.INSTRUMENTS}"
f"{imap_data_access.VALID_INSTRUMENTS}"
)
if args.data_level not in imap_processing.PROCESSING_LEVELS[args.instrument]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also use valid data level from imap-data-access?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at this, and the VALID_PROCESSING_LEVELS is generic over in that repository and not "per instrument" like we are checking here. So I held off on that for now.

I agree though, we should look at making that update, but I think we should push it off to a follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that sounds good!

imap_processing/codice/cdf_attrs.py Outdated Show resolved Hide resolved
@tech3371
Copy link
Contributor

Looks great. Thank you so much for doing this!

@greglucas
Copy link
Collaborator Author

OK, I just pushed up one more commit with a bunch of TODOs that will hopefully help everyone with what the previous naming of the file/descriptor was to know what to go back to once we update the global attributes.

Copy link
Collaborator

@bourque bourque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! And thanks for making the changes for IDEX and CoDICE!

@greglucas greglucas merged commit c933ee3 into IMAP-Science-Operations-Center:dev Mar 19, 2024
17 checks passed
@greglucas greglucas deleted the filename-update branch March 19, 2024 13:57
laspsandoval pushed a commit to laspsandoval/imap_processing that referenced this pull request Apr 2, 2024
…filename-update

MNT: Update file naming convention via imap-data-access upgrade
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants