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

Move MAG L1A to CDF attribute manager #693

Merged
merged 45 commits into from
Jul 25, 2024

Conversation

maxinelasp
Copy link
Contributor

Change Summary

Since the MAG code has 4 files with similar attributes but different data, this is kind of a weird change. I rearranged some pieces to try and reduce the overall complexity.
The attribute manager mostly made it simpler.

Overview

  • Add L1A YAML for MAG (This is mostly the same as L1B with a few additional variables)
  • Update L1A code to pass around attribute manager rather than the weird attribute management method from before
  • Fix CLI returns
  • group together primary/secondary data to make it more clear what is coming from where and why

Deleted Files

  • mag attrs file
    • no longer needed as all MAG processing uses YAML now

Sorry, I know this PR is a little complex, but I couldn't really find a good way of splitting it up as I was halfway through a refactor when we switched CDF management systems...

@maxinelasp maxinelasp requested a review from a team July 10, 2024 21:49
@maxinelasp maxinelasp self-assigned this Jul 10, 2024
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.

I'm still learning OOP so I'll leave it up to someone else to Approve. Looks good to me though. Some minor suggestions.

imap_processing/mag/constants.py Show resolved Hide resolved
imap_processing/mag/constants.py Show resolved Hide resolved
class Sensor(Enum):
"""Enum for MAG sensors: raw, MAGo, and MAGi (RAW, MAGO, MAGI)."""

MAGO = "MAGO"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MAGO = "MAGO"
MAGO = "MAGO" # MAGo sensor for outer measurements

imap_processing/mag/constants.py Show resolved Hide resolved
imap_processing/mag/constants.py Show resolved Hide resolved
imap_processing/mag/l0/decom_mag.py Show resolved Hide resolved
imap_processing/mag/l1a/mag_l1a.py Show resolved Hide resolved
imap_processing/mag/l1a/mag_l1a_data.py Show resolved Hide resolved
imap_processing/mag/l1a/mag_l1a_data.py Outdated Show resolved Hide resolved
@maxinelasp maxinelasp requested review from a team, bourque, sdhoyt, subagonsouth and tech3371 and removed request for a team July 17, 2024 19:37
@anamanica
Copy link
Contributor

Hi Maxine! I obviously shouldn't be the one to approve this, but I think this looks awesome. Your code for the CdfAttributes class is really awesome, it makes the code look super clean (just like Laura said).

imap_processing/mag/l1a/mag_l1a.py Show resolved Hide resolved
imap_processing/mag/l1a/mag_l1a.py Outdated Show resolved Hide resolved
imap_processing/mag/l1a/mag_l1a_data.py Outdated Show resolved Hide resolved
@greglucas
Copy link
Collaborator

Hi Maxine! I obviously shouldn't be the one to approve this, but I think this looks awesome. Your code for the CdfAttributes class is really awesome, it makes the code look super clean (just like Laura said).

@anamanica I don't see an issue with you approving this. If you agree with it, feel free to put your stamp of approval on it :)

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.

I tried to look at CDF attrs in details but looked at dataclasses on high level. Let me know if you like me to look in detail at any algorithm related changes. CDF stuff looks great with minor comment.

imap_processing/mag/l1a/mag_l1a.py Show resolved Hide resolved
imap_processing/mag/l1a/mag_l1a_data.py Show resolved Hide resolved
@maxinelasp maxinelasp mentioned this pull request Jul 25, 2024
@maxinelasp maxinelasp merged commit 50302ec into IMAP-Science-Operations-Center:dev Jul 25, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants