Glows Decom work by maxinelasp · Pull Request #175 · IMAP-Science-Operations-Center/imap_processing

maxinelasp · 2023-09-27T21:24:55Z

Change Summary

Adding files for GLOWS L0 decom work, issue #104 and part of #105.

Overview

I created a data class for both direct event and histogram data in glows. This code will decommute the test packet provided by the GLOWS team, and return two lists of the two main science packets needed for processing.

This review includes both the separate XML files for both direct events and histograms, and a combined XML file for both of them. The final code uses only the combined file. These files were almost entirely generated from the xtce_generator. I am planning on going back and adding both the binary data type, which I manually changed for the histogram data field, and code for creating multi-APID xml files.

New Files

glows/l0/glows_decom
- Provides a method for decoding GLOWS packets into histogram and direct events data
glows/l0/glows_l0_data
- Provides data classes for GLOWS level 0 data, including methods for modifying that data (I expect to need some additional methods for processing before outputting to CDF files, so although they're overkill now, this is future-planning)
packet definitions
- 3 packet defintions files: one each for histogram and direct events data, and one which combines them
test_glows_decom
- tests for glows_decom.py
xtce_generation/xtce_generator_glows
- code for automatically generating the separate histogram and direct events XTCE files

Updated Files

xtce_generator.py
- Updating to fix a typo in the unsigned integer generation

Testing

I added some general decom unit tests. This uses a smaller piece of the whole packet file, since the entire thing is very large.

In addition, I was able to confirm the histogram data matches the expected value from the packets using a plot provided to me by the GLOWS team:

Expected result:

My results:

… template.

…ting documentation and tests

maxinelasp · 2023-09-27T21:26:40Z

+                    de_l0 = GlowsDeL0(packet)
+                    dedata.append(de_l0)
+
+        except ReadError as e:


This is similar to the safe_packet_generator that Laura created in her Ultra L0 PR: #128. It does occur with the smaller packet that I created, because I ended up cutting it off in the middle of the last packet.

It does occur with the smaller packet that I created, because I ended up cutting it off in the middle of the last packet

I think this was covering up a true error in Ultra, so I'd be hesitant to put this in here. Are you able to trim the packet to the exact binary length you want? Maybe also put the command you did to trim the file in a comment at the top of this file or somewhere so that we can remember how to recreate the data here if we get a new test data dump.

Yeah, I'll take a look at the packet and see if I can actually get a properly trimmed packet. I was using the code that the GLOWS team provided, which does byte by byte reading, but apparently failed to actually chop it at the right place. I'll try again, just wanted to get this PR up mostly

Agree with Greg. Glad he caught that for Ultra.

maxinelasp · 2023-09-27T21:28:31Z

+			<xtce:Parameter name="HISTOGRAM_DATA" parameterTypeRef="BYTE28800">
+				<xtce:LongDescription>Histogram Counts</xtce:LongDescription>
+			</xtce:Parameter>
+			<!--Direct events sequence containers-->


There is a name collision in the histogram and direct events packets - they both have a parameter named SEC. I could update the name (DE_SEC) but opted to just use the same SEC definition (line 81) because they were the same size and represent the same thing. This may come up again with other combined XTCE files.

I think you highlight a great observation here. Something to keep in mind for sure. The xtce looks great!

It's ok to use same parameter if they do same thing and same size but referred in different packets. I have similar thing in SWE.

tech3371 · 2023-09-28T20:16:57Z

+			<xtce:Parameter name="HISTOGRAM_DATA" parameterTypeRef="BYTE28800">
+				<xtce:LongDescription>Histogram Counts</xtce:LongDescription>
+			</xtce:Parameter>
+			<!--Direct events sequence containers-->


It's ok to use same parameter if they do same thing and same size but referred in different packets. I have similar thing in SWE.

tech3371 · 2023-09-28T20:41:24Z

+    hist_packet_definition = xtcedef.XtcePacketDefinition(xtce_document)
+    histparser = parser.PacketParser(hist_packet_definition)
+
+    histdata = []
+    dedata = []
+
+    with open(packet_file_path, "rb") as binary_data:
+        try:
+            hist_packets = histparser.generator(
+                binary_data,
+                buffer_read_size_bytes=5790778,
+            )


We do lot of these in imap_processing/decom.py. We could update that function to take optional parameter buffer_read_size_bytes in case other needs to use it in the future.

I didn't use that method because I wanted to use the generator directly and produce the data types as needed, rather than reading and converting the entire packet, converting that to a list, then looping through the list again to process it for glows.

tech3371 · 2023-09-28T20:57:17Z

+    def __init__(self, packet):
+        self.data = self.process_packet(packet)
+
+    def process_packet(self, packet) -> dict:


What is the reason for this function? It seems to be doing duplicate work that may not be needed. What I mean is packet already has keys and value of HIST packet. You can reference it like this
Eg.

for key, value in packet.data.items(): if key != "HISTOGRAM_DATA": # get key and value here print(key, value.derived_value)

This is a great point. My one concern is, the expected list of keys does act as an additional check on the data - if one of the expected keys is missing, then the program raises an error. Is that something we would ever expect to happen? Or is that already checked by space_packet_parser?

I went ahead and made the change, but I'm still curious about the answer to this question.

I see. I see your point. I think that's where validation data comes to help. The way you validated with the plot. If the packet definition didn't have a key, validation data wouldn't match. But if we track that in here too, then it would add more things to track. That's my thought :)

tech3371 · 2023-09-28T21:01:57Z

+                raise ValueError(f"Missing data for {key} in {packet.data}")
+        return data
+
+    def _convert_histogram_data(self, binary_hist_data: str) -> list[int]:


can we replace this function with this line? I did this in SWE. SWE also needed to get data in 8-bit chunks too.

binary_hist_data = packet.data["HISTOGRAM_DATA"].raw_value byte_data = int(binary_hist_data, 2).to_bytes(3599, byteorder="big") # convert bytes to numpy array of uint8 raw_counts = np.frombuffer(byte_data, dtype=np.uint8)

No, that doesn't work. I get an overflow error from trying to convert the binary_hist_data to an int.

tech3371 · 2023-09-28T21:02:26Z

+            Data dictionary created from the packet, using L0_KEYS
+        """
+        data = {}
+        for key in self.L0_KEYS:


same comment as above

…cket

Co-authored-by: Tenzin Choedon <36522642+tech3371@users.noreply.github.com>

greglucas

Approving modulo fixing the false error.

greglucas · 2023-09-29T14:44:28Z

                )
                parameter_type.attrib["name"] = parameter_type_ref_name
-                parameter_type.attrib["signed"] = "false"
+                parameter_type.attrib["signed"] = false


Suggested change

parameter_type.attrib["signed"] = false

parameter_type.attrib["signed"] = False

Is false defined above to be a string? If so it is confusing. Or do you want this to be the boolean and need the caps?

It should be a string, I'll update that.

sdhoyt

Looks great to me!

laspsandoval · 2023-10-02T15:12:56Z

+			<xtce:BinaryParameterType name="BYTE28800">
+				<xtce:BinaryDataEncoding bitOrder="mostSignificantBitFirst">
+					<xtce:SizeInBits>
+						<xtce:FixedValue>28800</xtce:FixedValue>


Wondering if this might change and not be a FixedValue.

Good, point, as far as I am aware, it is a fixed value. I have gotten some test data and this successfully decoms it. It's a standard size for the number of histogram buckets, so I think it will stay the same, but I will double-check when I meet with them next.

With Ultra there were some nuances that I didn't figure out until I was given the Flight Software Specifications Table. Even after that we had to walk through their L0 decom code in C for a couple of hours and only then it became clear what was going on. My suggested questions for them:

Should the decom data be exactly the same as the simulator data (I do think that it should be)

Should this be a variable length packet?

Do they take only the first x values from the returned array? Ultra had hard-coded [0:48] into their C code to select only the first 48 values, which I never would have known about if I hadn't been walked through what they were doing.

I might just be paranoid from my own experience so maybe its all exactly as it should be :) I just recommend that you double check.

Definitely will! Thank you for those suggested questions, that's helpful for understanding your concerns and what I should look into.

I agree that this is a good thing to discuss with them, but I would tend to agree that this is almost certainly a fixed value. I think this comes down to differences between whether instruments detect individual events, or if they are integrating over some bins. So, I think the event-based instruments will accumulate individual events and therefore we don't know how long those packets will be until we get them. But, for the other instruments, like GLOWS, they will be integrating over some specific bin and just adding up those counts within those bins, so the number of bins is fixed and they just report the number of events/bin, not individual events. That is my guess at the reason for the fixed vs variable-length differences between instruments.

laspsandoval · 2023-10-02T15:23:47Z

+    SWVER : int
+        Version of SW used for generation
+    SEC : int
+        Block start time (IMAP), seconds<


minor typo <

laspsandoval · 2023-10-02T15:31:48Z

Maybe it is my imagination, but the Expected Result and your result look ever so slightly different. I do wonder if the Fixed value parameter in the xtce may have something to do with it. Just for the sake of not missing anything have you considered adding more unit tests?

All the code looks good.

maxinelasp · 2023-10-02T15:48:49Z

Maybe it is my imagination, but the Expected Result and your result look ever so slightly different. I do wonder if the Fixed value parameter in the xtce may have something to do with it. Just for the sake of not missing anything have you considered adding more unit tests?

Like I mentioned as a reply to one of the other comments, the GLOWS team and I have discussed having some statistical tests for this data, so that's my ultimate plan to test the data. I think the graph provided to me by the team is the data that went into the simulator, so I would expect some minor differences. I'll definitely talk about testing this process with them when I meet with them later this month.

Since the histogram value (with the FixedValue) should always have the same number of buckets, I think a FixedValue is the right way to go there. Unless part of the packet is missing, there should always be the same bucket count. I will confirm this with GLOWS.

maxinelasp · 2023-10-02T22:37:15Z

I'm merging this with the code coverage check failing, because the uncovered lines do not need testing (pretty print methods in the two data classes)

* update telemetry_generator to be more generic * first pass at glows xtce * updated generator to use correct IntegerParameterType * First pass at GLOWS decom * Updating glows decom to properly process data into dictionary * Updating decom to include direct events * Finalizing code, adding smaller test data file and combined XML, updating documentation and tests * Updating data classes to use attributes, and changing names * Removing keys from data class and getting attribute names from the packet * Move INT to SINT in telemetry_generator Co-authored-by: Tenzin Choedon <36522642+tech3371@users.noreply.github.com> * Addressing code review comments --------- Co-authored-by: Tenzin Choedon <tenzin.choedon@lasp.colorado.edu>

maxinelasp and others added 15 commits September 8, 2023 16:29

update telemetry_generator to be more generic

37d3b34

Updates to telemetry generator

ad4dedb

updating docs

3121fe1

updating to remove local-specific testing changes

dd7d879

first pass at glows xtce

56ad948

update way of checking if description exists or not. update generator…

be2e29b

… template.

updated generator to use correct IntegerParameterType

9fcc578

Merge branch 'xtce_fixes' into glows_l1

4493830

Merge branch 'upstream-dev' into glows_l1

4c7222f

First pass at GLOWS decom

cb33d3b

updating telemetry generator

d949760

Updating glows decom to properly process data into dictionary

93b8c10

Finished histogram decom

666474e

Updating decom to include direct events

be9c43b

Finalizing code, adding smaller test data file and combined XML, upda…

88c0961

…ting documentation and tests

maxinelasp added the Ins: GLOWS Related to the GLOWS instrument label Sep 27, 2023

maxinelasp requested review from a team September 27, 2023 21:24

maxinelasp self-assigned this Sep 27, 2023

maxinelasp requested review from GFMoraga, bourque, bryan-harter, greglucas, laspsandoval, sdhoyt and tech3371 and removed request for a team September 27, 2023 21:24

maxinelasp commented Sep 27, 2023

View reviewed changes

Updating test file path

926f99e

updating docs

9a2f6bb

tech3371 reviewed Sep 28, 2023

View reviewed changes

maxinelasp and others added 5 commits September 28, 2023 15:28

Updating data classes to use attributes, and changing names

8d2acf9

Removing keys from data class and getting attribute names from the pa…

e953681

…cket

Move INT to SINT in telemetry_generator

dd81be8

Co-authored-by: Tenzin Choedon <36522642+tech3371@users.noreply.github.com>

Addressing code review comments

d6e30c8

Fixing test failure

7f28aa7

greglucas approved these changes Sep 29, 2023

View reviewed changes

maxinelasp added 2 commits September 29, 2023 09:03

Review updates

3c38dab

Merge branch 'upstream-dev' into glows_l1

6d7295f

maxinelasp force-pushed the glows_l1 branch 4 times, most recently from 1646c51 to 6d7295f Compare September 29, 2023 16:26

sdhoyt approved these changes Sep 29, 2023

View reviewed changes

tech3371 approved these changes Sep 29, 2023

View reviewed changes

laspsandoval reviewed Oct 2, 2023

View reviewed changes

maxinelasp force-pushed the glows_l1 branch from 0014522 to 585db72 Compare October 2, 2023 21:08

fixing typo

be03bea

maxinelasp force-pushed the glows_l1 branch from 585db72 to be03bea Compare October 2, 2023 21:10

maxinelasp and others added 2 commits October 2, 2023 15:17

Merge branch 'dev' into glows_l1

4483eab

Updating IntEnum to Enum to fix automatic documentation

69ac094

maxinelasp merged commit 6af8925 into IMAP-Science-Operations-Center:dev Oct 2, 2023

maxinelasp mentioned this pull request Oct 12, 2023

Glows L0 changes to data class to accomodate L1A needs #264

Merged

maxinelasp mentioned this pull request Nov 15, 2023

Create GLOWS XTCE Packet Definition #104

Closed

1 task

	parameter_type.attrib["signed"] = false
	parameter_type.attrib["signed"] = False

Uh oh!

Conversation

maxinelasp commented Sep 27, 2023

Change Summary

Overview

New Files

Updated Files

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greglucas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdhoyt left a comment

Choose a reason for hiding this comment

Uh oh!

laspsandoval Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laspsandoval commented Oct 2, 2023

Uh oh!

maxinelasp commented Oct 2, 2023

Uh oh!

laspsandoval Oct 2, 2023 •

edited

Loading