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 Direct Event Refactor #971

Merged
merged 17 commits into from
Oct 9, 2024

Conversation

sdhoyt
Copy link
Contributor

@sdhoyt sdhoyt commented Oct 7, 2024

Change Summary

Overview

This PR refactors the Lo L1A Direct Event algorithm from a data class to a function to later make use of the packets_to_dataset(). To reduce the PR size, the algorithm isn't yet called by the lo_l1a.py script. That will be done in a separate PR.

Closes #862

@sdhoyt sdhoyt added Ins: Lo Related to the IMAP-Lo instrument Level: L0 Level 0 processing Level: L1 Level 1 processing labels Oct 7, 2024
@sdhoyt sdhoyt added this to the Oct 2024 milestone Oct 7, 2024
@sdhoyt sdhoyt requested review from a team October 7, 2024 16:59
@sdhoyt sdhoyt self-assigned this Oct 7, 2024
@sdhoyt sdhoyt requested review from subagonsouth, laspsandoval, vmartinez-cu and maxinelasp and removed request for a team October 7, 2024 16:59
# The DE index for the entire pointing
pointing_de = 0
# for each direct event packet in the pointing
for pkt_idx, de_count in enumerate(dataset["count"].values):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer for this section to be in a different method - mainly for documentation. Overall it's pretty clear what's happening here, though! Is it possible to loop through the values of DATA_BITS rather than writing them all out here? You have a lot of dataset["{val}"] = parse_de_bin(dataset, pkt_idx, bit_pos, {val} that could reduce duplication. The last set of if statements can probably work the same way.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you do greatly simplify this for loop then it's fine in this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'll clean this up

Copy link
Contributor

@maxinelasp maxinelasp left a comment

Choose a reason for hiding this comment

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

LGTM thanks for tidying!

@sdhoyt
Copy link
Contributor Author

sdhoyt commented Oct 8, 2024

LGTM thanks for tidying!

@maxinelasp Thanks! I also just used your looping through the data bits fields idea and it's cleaned up even more now. Thanks for the comment!

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.

This all looks really good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really nice tests! Great job generating fake data that you can use for testing in a very deterministic way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@sdhoyt sdhoyt merged commit 8f1fef9 into IMAP-Science-Operations-Center:dev Oct 9, 2024
17 checks passed
@sdhoyt sdhoyt deleted the lo-de-refactor branch October 9, 2024 19:13
@tech3371 tech3371 removed this from the Oct 2024 milestone Oct 18, 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 Level: L0 Level 0 processing Level: L1 Level 1 processing
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Refactor Lo L1A Direct Event Data Class to Function
4 participants