-
Notifications
You must be signed in to change notification settings - Fork 12
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 compression #801
base: dev
Are you sure you want to change the base?
Mag l1a compression #801
Conversation
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
…essing into mag_l1a_compression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting concept. Only a few suggestions.
Nice job with your doc strings and comments. The details are helpful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Epic effort maxine, thank you!
I have left you lots of comments (sorry!). Lots are just style and readability stuff - i know how hard it is to make this compression logic comprehendable so I feel your pain.
Think my only other query is on error handling. I think it should be a bit more defensive and if you get any bad unexpected values it means something awful has happened and the whole packet should be considered a stream of NaNs and an error raised somewhere so we can investigate..
Thank you!
# the range | ||
uncompressed_vector_size = compression_width * 3 + 2 | ||
# plus 8 to get past the compression width and range data section | ||
first_vector_width = uncompressed_vector_size + 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont understand the +8 here - you are aready skipping 8 bits in bit array on line 546?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the index of the end of the first vector, so it's uncompressed_vector size long, but we need to skip past the first 8 bits - so the end index is uncompressed_size plus 8. I can move the +8
into the slice in line 546, if that makes more sense?
@alastairtree @tech3371 @sdhoyt @laspsandoval Thank you so much for your reviews! I have addressed all the comments on this PR, added some additional tests, and had the output validated by the MAG team. Could I get a re-review and maybe an approval from you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me. Nice work!
Change Summary
This is a pretty involved PR. I would like to thank in advance anyone who reviews. I don't think breaking this code up further would make sense, or I would have done so.
I'd recommend starting by reading the docstring for "process_compressed_vectors" in mag_l1a_data.py, as it gives a good overview on the overall algorithm.
Note: "process_uncompressed_vectors" was moved into a new method but otherwise unchanged.
Overview
Updated Files
mag_l1a_data.py
test_mag_l1a
mag_l0_data
Testing
Testing is pretty extensive for this change, since it's so complex. I generated test data primarily using a piece of code that the MAG team provided for me.
Closes issue #727