-
Notifications
You must be signed in to change notification settings - Fork 13
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
packet_file_to_datasets *NOT* in IDEX #743
packet_file_to_datasets *NOT* in IDEX #743
Conversation
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.
Thank you for trying to make this work! It sounds like you learned some things along the way so that is good!
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.
Looks good to me. Some very minor nits, that could be follow-on PRs if desired too. I think its great to add documentation about the limitations, so thank you for adding this!
imap_processing/utils.py
Outdated
Note: While this is a general function intended to work for all | ||
instruments, this cannot be used for IDEX due to the complex nature of | ||
the xml file. IDEX will continue to use PacketParser and RawDustEvents | ||
found in idex_l1.py. |
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 think this should be either a Notes
section in numpydoc, https://numpydoc.readthedocs.io/en/latest/format.html#notes or a .. note::
rst callout.
https://sublime-and-sphinx-guide.readthedocs.io/en/latest/notes_warnings.html
Some minor wordsmithing, where I'd avoid saying what we'll do with the specific classes in idex_l1.py
in case someone refactors that later and forgets to update this docstring.
Note: While this is a general function intended to work for all | |
instruments, this cannot be used for IDEX due to the complex nature of | |
the xml file. IDEX will continue to use PacketParser and RawDustEvents | |
found in idex_l1.py. | |
.. note:: | |
This function only handles packet definitions with the same variable structure across | |
all packets with the same ApId. For example, this cannot be used for IDEX due to the | |
conditional XML structure defined for their science packet. |
One other thought that occurred to me is whether we could catch this and print out a nice warning message here so others don't also hit this roadblock. Something like creating a new dictionary that keeps track of apid: variables
. Around l365 below I think something like this might work?
if apid not in data_dict:
# This is the first packet for this APID
data_dict[apid] = collections.defaultdict(list)
datatype_mapping[apid] = dict()
variable_mapping[apid] = packet.data.keys()
if packet.data.keys() != apid_variable_mapping[apid]:
raise ValueError(f"Cannot parse apid {apid} which has different variables defined across the same packet type")
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.
Sweet! Yeah, I agree with you. I changed the comment to a "Notes" section in the doc-string.
I didn't end up implementing the dimension check in this PR (I just don't quite understand the code yet, and I feel like I should before I add it and test it). Would you like me to write another issue ticket?
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.
Yeah, that'd be great. I think the code above is close to working, so if you copy that over to another issue and work from that it might be a good thing. Additionally, add a test trying to parse IDEX and showing that your ValueError
is raised with pytest.raises(ValueError, match="Cannot prase apid")
.
c71368e
into
IMAP-Science-Operations-Center:dev
Change Summary
Overview
Originally, this PR was intended to switch over IDEX to using the
packet_file_to_datasets
function established in utils.py, however it was decided that this would be more work than it is worth. Due to the nature of the.xml
file for IDEX, using this function created dimension issues. For now, IDEX will continue to use PacketParser, and RawDustEventsI added a comment in the doc-string for
packet_file_to_datasets
to explain this.New Dependencies
None
New Files
None
Deleted Files
None
Updated Files
packet_file_to_dataset
docstring.Testing
None
closes 687 for IDEX only!