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

Mag l1a processing #384

Merged
merged 13 commits into from
Apr 8, 2024
Merged

Conversation

maxinelasp
Copy link
Contributor

Change Summary

Finishing MAG L1A processing, including creating vectors and timestamping them. This still needs some work on the timestamps (they're still in spacecraft time and should be in J2000) and some CDF generation work.

Overview

Data classes are in mag_l1a_data. L1A processing is in mag_l1a_data and mag_l1a. I refactored some earlier work to split up normal and burst mode upon packet read, as these packets are not mixed together again and it makes the code cleaner. And moved from bytearray to numpy array in early L1a processing.

New Dependencies

None

New Files

  • mag_l1a_data
    • dataclasses for L1A

Updated Files

  • mag_l1a
    • Added L1A processing
  • mag_decom
    • Moved around some of the packet sorting steps and simplified processing

Testing

Added new tests for L1A, primarily using validation data provided by MAG. I plan to go back and add one more byte-processing test.

@maxinelasp maxinelasp requested a review from a team April 1, 2024 16:47
@maxinelasp maxinelasp self-assigned this Apr 1, 2024
@maxinelasp maxinelasp requested review from bourque, sdhoyt, greglucas, tech3371, vmartinez-cu and laspsandoval and removed request for a team April 1, 2024 16:47
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.

Very minor comments from me. Looks really nice overall!

imap_processing/mag/l0/mag_l0_data.py Outdated Show resolved Hide resolved
imap_processing/mag/l0/mag_l0_data.py Outdated Show resolved Hide resolved
imap_processing/mag/l1a/mag_l1a.py Outdated Show resolved Hide resolved
imap_processing/mag/l1a/mag_l1a.py Show resolved Hide resolved
imap_processing/mag/l1a/mag_l1a_data.py Outdated Show resolved Hide resolved
Comment on lines 121 to 129
sample_time_interval = 1 / self.vectors_per_second
previous_time = self.start_time
for index, vector in enumerate(self.vectors):
if index == 0:
new_vector = Vector(vector, self.start_time)
else:
new_vector = Vector(vector, previous_time + sample_time_interval)

previous_time = new_vector.timestamp
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can be simplified a bit by updating the time at the end of your loop iteration.

Suggested change
sample_time_interval = 1 / self.vectors_per_second
previous_time = self.start_time
for index, vector in enumerate(self.vectors):
if index == 0:
new_vector = Vector(vector, self.start_time)
else:
new_vector = Vector(vector, previous_time + sample_time_interval)
previous_time = new_vector.timestamp
sample_time_interval = 1 / self.vectors_per_second
current_time = self.start_time
for index, vector in enumerate(self.vectors):
new_vector = Vector(vector, current_time)
# Increment our time counter before the next iteration
current_time = new_vector.timestamp + sample_time_interval

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, that's way better

Comment on lines +179 to +188
if i % 4 == 0: # start at bit 0, take 8 bits + 8bits
# pos = 0, 25, 50...
x = (
((vector_data[pos + 0] & 0xFF) << 8)
| ((vector_data[pos + 1] & 0xFF) << 0)
) & 0xFFFF
y = (
((vector_data[pos + 2] & 0xFF) << 8)
| ((vector_data[pos + 3] & 0xFF) << 0)
) & 0xFFFF
z = (
((vector_data[pos + 4] & 0xFF) << 8)
| ((vector_data[pos + 5] & 0xFF) << 0)
) & 0xFFFF
rng = (vector_data[pos + 6] >> 6) & 0x3
pos += 6
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again minor (and I realize you got this from elsewhere), but I would probably write this with variables being parameterized and then used in the lookups a single time to try and avoid the duplication of those lookups. No need to change here unless you like this form better.

if i % 4 == 0:
    left_mask = 0xFF
    left_shift = 0
elif i % 4 == 1:
    left_mask = 0x3F
    left_shift = 2
...
x = ((vector_data[pos + 0] & left_mask) << (left_shift + 8))
        | (vector_data[pos + 1] & 0xFF) << left_shift))
       & 0xFFFF

imap_processing/tests/mag/test_mag_decom.py Show resolved Hide resolved
@maxinelasp
Copy link
Contributor Author

pre-commit.ci autofix

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.

minor question but looks great!

imap_processing/mag/l0/mag_l0_data.py Show resolved Hide resolved
imap_processing/mag/l0/mag_l0_data.py Show resolved Hide resolved
imap_processing/mag/l1a/mag_l1a.py Show resolved Hide resolved
imap_processing/mag/l1a/mag_l1a.py Outdated Show resolved Hide resolved
# byte boundaries are.

# VECSEC is already decoded in mag_l0
total_primary_vectors = seconds_per_packet * mag_l0.PRI_VECSEC
Copy link
Contributor

Choose a reason for hiding this comment

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

love these descriptive variable names! Makes it easy to follow

----------
vector_data : np.ndarray
Raw vector data, in bytes. Contains both primary and secondary vector data
(first primary, then secondary)
Copy link
Contributor

Choose a reason for hiding this comment

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

As usual, great comments!

# chunk corresponds to 3 16 bit signed integers. The last 2 bits are the sensor
# range.

for i in range(primary_count + secondary_count): # 0..63 say
Copy link
Contributor

Choose a reason for hiding this comment

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

love the side comments!

@maxinelasp maxinelasp merged commit acd380d into IMAP-Science-Operations-Center:dev Apr 8, 2024
15 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.

3 participants