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

Lo L1A Histogram packet_to_dataset() Refactor and CDF Creation #788

Merged
merged 26 commits into from
Sep 18, 2024

Conversation

sdhoyt
Copy link
Contributor

@sdhoyt sdhoyt commented Aug 27, 2024

Change Summary

Overview

This PR refactors the the Lo L1A histogram code to use the packet_to_dataset() function and adds code to create the L1A CDF. There ended up being more changes that I had hoped, so I can try to break this up if needed. What I was doing on this branch evolved a bit over time, so this isn't a compartmentalized as a should be.

New Dependencies

None

New Files

  • lo_science.py
    • new L1A science processing file. Currently only contains histogram processing

Deleted Files

  • science_counts.py
    • science counts data class not needed with packets_to_dataset()
  • lo_data_container.py
    • data container for data classes no longer needed with packets_to_dataset()
  • lo_l1a_write_cdfs.py
    • Planning to set CDF attributes in lo_l1a.py main processing file. No longer need this file.

Updated Files

  • imap_lo_l1a_variable_attrs.yaml
    • renamed tof histogram attributes so they don't conflict with DE tof attributes
  • cli.py
    • removed list around processing function return
  • lo_apid.py
    • removed needed APIDs
  • lo_base.py
    • updated space packet parser import. This file will eventually be removed once everything is converted to use packets_to_dataset.
  • lo_l1a.py
    • Refactored to use packets_to_dataset, removed temporary code from SIT-3, and added cdf attribute creation for L1A hist
  • lo_xtce.xml
    • fixed name of sci count field.

Testing

@sdhoyt sdhoyt self-assigned this Aug 27, 2024
Copy link
Collaborator

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

Leaving some initial feedback for now, likely won't be able to review again for a while, so don't wait for my final review/approval.

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.

Looks good to me. I left one nit pick on naming. Ping me for review again once you address some of gregs comments.

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.

A few comments/suggestions.

@subagonsouth
Copy link
Contributor

@sdhoyt, it is not clear to me what comments have been addressed or that you feel are resolved. Can you mark what you think is done as resolved and then ping me and I will review.

@sdhoyt
Copy link
Contributor Author

sdhoyt commented Sep 13, 2024

@sdhoyt, it is not clear to me what comments have been addressed or that you feel are resolved. Can you mark what you think is done as resolved and then ping me and I will review.

@subagonsouth All the comments are now marked as resolved. Most of the CDF YAML related comments are resolved by the creation of this issue: #843. I'm going to make improvements to the YAML in a separate PR.

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.

Sorry for the slow feedback.

@sdhoyt sdhoyt merged commit 7f471b9 into IMAP-Science-Operations-Center:dev Sep 18, 2024
17 checks passed
@sdhoyt sdhoyt deleted the lo-hist-redo branch September 18, 2024 14:49
@bourque bourque added the Ins: Lo Related to the IMAP-Lo instrument label Sep 23, 2024
@bourque bourque added this to the Sept 2024 milestone Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ins: Lo Related to the IMAP-Lo instrument
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants