-
Notifications
You must be signed in to change notification settings - Fork 16
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
MNT: space_packet_parser update #1007
Changes from 18 commits
51c9d82
3fceeba
8dd16f9
9e4ca1c
5085029
b43f2b3
c3e9c4d
bd3dcc9
f11f4e7
e7cda0c
a3a5676
8a7c9c7
6ded7f3
f9115f7
47742f2
40cefa5
5509ad1
b2f5e8c
d2715af
9be6e9a
65ddad6
a598d72
666e9b4
3b83380
137e506
1066ee3
2082ea5
3cb1995
a20bcab
43f6980
4894653
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @laspsandoval After reverting some of
I did make changes to this file again since it seems like you did want your data in derived value. Please do revisit this in future PR if that's not what you want. It seems to involve more changes and I am worried I will mess something. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now I understand why it failed last time. It needed |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,7 +65,7 @@ def add_metadata_to_array(packet: space_packet_parser, metadata_arrays: dict) -> | |
|
||
Parameters | ||
---------- | ||
packet : space_packet_parser.parser.Packet | ||
packet : space_packet_parser.packets.CCSDSPacket | ||
CODICE data packet. | ||
metadata_arrays : dict | ||
Metadata arrays. | ||
|
@@ -88,7 +88,7 @@ def add_metadata_to_array(packet: space_packet_parser, metadata_arrays: dict) -> | |
for key, value in packet.header.items(): | ||
metadata_arrays.setdefault(key, []).append(value.raw_value) | ||
|
||
for key, value in packet.data.items(): | ||
for key, value in packet.user_data.items(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, are the types here correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we check with Gavin? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to get clarification from him There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't this be removed entirely because it is in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. you are right Greg. I can delete this function. It's not used anymore. Sorry for including you to this PR by mistake. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To your question Maxine, data type of both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But what is inside the dicts? Python types? I did go look at that code but I'm still confused about the parameter types There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may be a Gavin question :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. subclasses of built-in types. An |
||
if key not in ignore_list: | ||
metadata_arrays.setdefault(key, []).append(value.raw_value) | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3,7 +3,7 @@ | |||||
from enum import Enum | ||||||
from pathlib import Path | ||||||
|
||||||
from space_packet_parser import parser, xtcedef | ||||||
from space_packet_parser import definitions | ||||||
|
||||||
from imap_processing import imap_module_directory | ||||||
from imap_processing.ccsds.ccsds_data import CcsdsData | ||||||
|
@@ -49,39 +49,28 @@ def decom_packets( | |||||
f"{imap_module_directory}/glows/packet_definitions/GLX_COMBINED.xml" | ||||||
) | ||||||
|
||||||
packet_definition = xtcedef.XtcePacketDefinition(xtce_document) | ||||||
glows_parser = parser.PacketParser(packet_definition) | ||||||
packet_definition = definitions.XtcePacketDefinition(xtce_document) | ||||||
|
||||||
histdata = [] | ||||||
dedata = [] | ||||||
|
||||||
filename = packet_file_path.name | ||||||
|
||||||
with open(packet_file_path, "rb") as binary_data: | ||||||
glows_packets = glows_parser.generator(binary_data) | ||||||
glows_packets = packet_definition.packet_generator(binary_data) | ||||||
|
||||||
for packet in glows_packets: | ||||||
apid = packet.header["PKT_APID"].derived_value | ||||||
apid = packet.header["PKT_APID"] | ||||||
tech3371 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
# Do something with the packet data | ||||||
if apid == GlowsParams.HIST_APID.value: | ||||||
values = [ | ||||||
item.derived_value | ||||||
if item.derived_value is not None | ||||||
else item.raw_value | ||||||
for item in packet.data.values() | ||||||
] | ||||||
values = [item.raw_value for item in packet.user_data.values()] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before this was getting the derived value if it is not None. The I'm pretty sure this is the equivalent to before now:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maxinelasp what do you think here? I thought same as Greg but you helped me debug some of GLOWS' error and I saw that you had to get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, when I printed out the types here it was all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. class IntParameter(int):
|
||||||
hist_l0 = HistogramL0( | ||||||
__version__, filename, CcsdsData(packet.header), *values | ||||||
) | ||||||
histdata.append(hist_l0) | ||||||
|
||||||
if apid == GlowsParams.DE_APID.value: | ||||||
values = [ | ||||||
item.derived_value | ||||||
if item.derived_value is not None | ||||||
else item.raw_value | ||||||
for item in packet.data.values() | ||||||
] | ||||||
values = [item for item in packet.user_data.values()] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Missed that one! Sorry about that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comment above. I think this shouldn't be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was giving me errors before because it was assigning everything as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nvm, you did explain here. |
||||||
|
||||||
de_l0 = DirectEventL0( | ||||||
__version__, filename, CcsdsData(packet.header), *values | ||||||
|
tech3371 marked this conversation as resolved.
Show resolved
Hide resolved
|
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 we still need to get
value.raw_value
here... won't value be of typepackets.IntParameter
instead ofint
?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.
If I understood the original if statement, it seems like we want to get derived value if it's available or else get raw value. Now with new changes, we get derived value if it's available or else we get raw value. so getting the
value
will do what the if statement was doing before, if I understood correctly. That's how I understood at least when Gavin explain in the Slack thread. should we double check to be safe?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.
If you could just print out the type and validate that it's a python type, that's good enough for me
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.
packets.IntParameter
is a subclass ofint
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.
in this case, are we good with current change?
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.
Well, if it's a different type, that's going to mess with some code, right? Some of my code depends on the types being accurate because of MyPy
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 guess, if you make the change and all the tests pass, then it's probably fine... but I'm just worried about changing all the types out from underneath everyone like this, even if they're basically the same as the original types. But perhaps that is just Python offending my delicate, type-loving sensibilities again
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.
Sure, then I think you can change the types to
IntParameter
now. But note thatraw_value
is not the same asderived_value
which was being used before if it was present.