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

updates to utils create_dateset function #532

Merged
merged 3 commits into from
Apr 26, 2024

Conversation

tech3371
Copy link
Contributor

Change Summary

Overview

Updated imap_processing/utils.py's create_dataset function to be more generic and updated IMAP-Hi Housekeeping processing to use it.

Updated Files

Two main files that was changed

  • imap_processing/utils.py
  • imap_processing/hi/l1a/housekeeping.py

@tech3371 tech3371 added enhancement New feature or request Ins: Hi Related to the IMAP-Hi instrument labels Apr 24, 2024
@tech3371 tech3371 requested a review from a team April 24, 2024 16:24
@tech3371 tech3371 self-assigned this Apr 24, 2024
@tech3371 tech3371 requested review from bourque, sdhoyt, greglucas, subagonsouth, vmartinez-cu, laspsandoval and maxinelasp and removed request for a team April 24, 2024 16:24
Copy link
Contributor

@subagonsouth subagonsouth left a comment

Choose a reason for hiding this comment

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

Great so see some work to make a function more generic and reusable.

@@ -101,17 +101,22 @@ def convert_raw_to_eu(dataset: xr.Dataset, conversion_table_path, packet_name):
return dataset


def create_dataset(packets: list[Packet], met_name="shcoarse") -> xr.Dataset:
def create_dataset(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooooo. I didn't know about this function. Love that it is being made more generic. If I am not mistaken it can be used for any product that is generated directly from the packet storing everything in one-dimensional, 12-bit CDF variables? Some details about usage restrictions should be added to the docstring.

I wonder if functionality could be added to allow for custom unpacking of certain keys in the input packets as well as CDF variables that are not type "I12"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see now that I was misinterpreting the format="I12" and now understand that this is to display an integer with a field width of 12. Ignore that part of my comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice improvements to this function!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@subagonsouth let me know if you like clarification on this.

yes. I was trying to make this function more generic as I go along.

include_header: bool, Optional
Whether to include CCSDS header data in the dataset
skip_keys: list, Optional
Keys to skip in the metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

What is metadata in this context? I think it means that matching fields in the input packets are not included in the output dataset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, I am using metadata as keys from packet definition after CCSDS header keys. I appreciate other term suggestion.

imap_processing/utils.py Outdated Show resolved Hide resolved
Parameters
----------
packets : list[Packet]
packet list
met_name : str
metadata name to use as epoch time
met_name : str, Optional
Copy link
Contributor

Choose a reason for hiding this comment

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

Do some packets contain a value that is valid to directly use as the epoch coordinate? The few examples that I have seen require a function that converts what is the packet to what is desired to be stored in the epoch coordinate. For example, a SPICE call to convert CCSDS_MET to a datetime object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this phase, I am not converting because some uses this exact key to sort data by this time. May be we should change time here in the format the CDF desire(datetime64 object). I could do that in upcoming PR.

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.

I like how create_dataset has evolved over the last several months 😄

imap_processing/hi/l1a/housekeeping.py Show resolved Hide resolved
imap_processing/utils.py Outdated Show resolved Hide resolved
@@ -101,17 +101,22 @@ def convert_raw_to_eu(dataset: xr.Dataset, conversion_table_path, packet_name):
return dataset


def create_dataset(packets: list[Packet], met_name="shcoarse") -> xr.Dataset:
def create_dataset(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice improvements to this function!

imap_processing/utils.py Outdated Show resolved Hide resolved
@tech3371
Copy link
Contributor Author

I am going to merge this so that others can use it sooner. We can update this more in future PR as we go along.

@tech3371 tech3371 merged commit 0eee306 into IMAP-Science-Operations-Center:dev Apr 26, 2024
15 checks passed
@tech3371 tech3371 deleted the hi_update branch July 25, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Ins: Hi Related to the IMAP-Hi instrument
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants