This looks great!

It would be good to add a docstring here explaining what this class does, what attributes it has, what methods.

Right now, everything happens within instantiation which seems like we don't really need a class, but possibly just a function that returns the data you want? Perhaps there is a separate class IDEXEvent that stores event information, or IDEXWaveform for the waveform data, where you'd be creating these dataclasses as you process a packet. So, the actual processing of a file would be a function call like in SWE:

def decom_idex(...):
    list_of_idex_events = []
    for pkt in idex_packet_generator:
        # instantiate your event classes and fill them in as needed
        evt = IDEXEvent(pkt)
        list_of_idex_events.append(evt)
    return list_of_idex_events

We should decide how we want to architect these for consistency across instruments though.

Yeah everything interesting basically happens inside init() right now. I actually had several other methods here before which may or may not make it worth it to keep it as a class

• plot_waveforms: A function to quickly plot the data in the self.data structure in matplotlib as a quicklook to verify accuracy (and this might even be extended into a quicklook product in the future? Or maybe we want to quicklook directly from the L1a)
• create_l1a_cdf: the method called to populate data into an xarray object, and then convert it to cdf

Can you add high level explanation of what this class is doing? It would be helpful.

Is your waveforms data associated with whatever packet file came in, or with an individual event/packet? I guess I'm getting at what level of granularity do we want for the class.

I think you could put those methods you describe onto the IDEXEvent class, where it would be plotting a waveform from each event, storing what type of science data it was in this event, etc...? I haven't looked super close, but that is just what my mind goes to. Or even separate functions within the module, idex.plot_waveforms(pass_in_data)

We are going to get dumps from the POC every 3 days, which means a packet file may be 3-days long I think? Or partial days, etc... I'm not sure we want our CDF creation lined up with those semi-arbitrary boundaries, but rather we could have a separate create_l1a_cdf(list_of_packets) function that takes in all of the events you want going into your l1a CDF and not necessarily tied to this current class.

I won't block on this, but I do think it is important to think about because you're the first and sort of setting up the template for others to follow. It looks like others like this large class though, so maybe I'm the odd one out 😄

Yes a docstring is incoming! I should have included that first thing, sorry.

As far as the structure of the class, to my knowledge the L0 data we get from the MOC/POC will be sliced up grouped up into packets that span a single day, midnight to midnight UTC. So if there are 3 days between contacts, we should receive 3 separate L0 files. The CDF files that we generate should also span a single day, so there should be a 1-to-1 correspondence from a packet file to a l1a CDF file.

The waveforms are associated with a single dust event, so perhaps the class could be sub-divided into specific events rather than an entire L0 file (for everyone's reference, a single "event" in this context is a dust impact, which may occur roughly ~1-10 times a day). My concern with only looking at the files by a single event is:

• I don't know if the packets within the L0 file are required to be grouped by event, so we may end up populating a bunch of classes at once. Right now the packets are all time-ordered, but their algorithm document specifies that technically we should be looking at the AID of each packet to ensure we're filling in data to the right event.
• I think it's helpful to group all of the events together like this because this collection of events is ultimately what will make the l1a CDF file
• If we're hoping to keep the structure of the decom consistent across all of the instruments, IDEX would look like the oddball of the bunch if it grouped data by event rather than by a time range. I believe none of the other instruments are event based, and just continuously take data.

If we wanted, we could make DustEvent classes that this decom class uses to populate, but that might be a little overkill. Overall, the data we need to get out of the packet is pretty simple (just 6 1D arrays).

The CDF files that we generate should also span a single day, so there should be a 1-to-1 correspondence from a packet file to a l1a CDF file.

I'm not sure we can count on this... I think we can hope for it, but I would think that if a contact comes down at noon and they get data up until noon I'd expect them to deliver the files through noon and not wait until 3 days from then during the next contact to send down a complete days worth of data. That is a good question for how we want things to happen because I think we can somewhat dictate this.

I think it's helpful to group all of the events together like this because this collection of events is ultimately what will make the l1a CDF file

Fair enough, I think I was viewing Events as the critical assembly unit and CDF files being something we put together, where the event is the logical structure.

If we wanted, we could make DustEvent classes that this decom class uses to populate, but that might be a little overkill. Overall, the data we need to get out of the packet is pretty simple (just 6 1D arrays).

I think I'm just not being very clear with what is in my mind because I actually think it would be easier to parse (IMO) what is going on here rather than going to nested dictionaries with appended lists to try and get at the events (i.e. those 6 1D arrays would be stored in the DustEvent class. The datastore would then just work on this list of daily events.

daily_events = []
for packet in packet_dump:
    # DustEvent would be able to process any event and handle the logic for what science type it is
    # how to parse waveforms, etc...
    event = DustEvent(packet)
    daily_events.append(event)

# How many events?
len(daily_events)

# Filter by events of a certain type
len([event for event in daily_events if event.science_type == "COOL_SCIENCE_MODE"])

# Now further down if you want to make a CDF file with daily_events:
create_idex_cdf_l1a(daily_events)

# if you want you could have another class just to store groupings of events too
class DailyDustEvents:
    def __init__(events):
        self.events = events

    def create_l1a(self):
        pass

I'll experiment with the classes a little more, it probably will make it more readable to do it like you have with DailyDustEvents and DustEvent.

As for

I'm not sure we can count on this... I think we can hope for it, but I would think that if a contact comes down at noon and they get data up until noon I'd expect them to deliver the files through noon and not wait until 3 days from then during the next contact to send down a complete days worth of data. That is a good question for how we want things to happen because I think we can somewhat dictate this.

That does happen frequently on MAVEN and EMM, but then the next downlink we receive "v02" of the pkts file and processing starts anew. Typically that means that v01-r00 of the file might be incomplete, but the next revision probably contains the full amount of data. I don't know what we want to do on this mission, but thats basically just what I was assuming would happen.

We should use a logger here instead of prints, and add DEBUG log level if they aren't necessarily needed but helpful when we want to debug things.

Sounds good

This is more than logging at this point by assigning to attributes. Do we need these attributes added to the class, or can they just be local variables?

Ah yeah I think it's possible they could be added at some point, but I haven't figured out if they actually want that stuff in the CDF. My guess is they won't want it, so I'll remove the "self" stuff

We should try and stick to python variable naming conventions when possible.

You aren't accessing self at all, so this could be a static method on the class, but again this seems like a helper function within this module so isn't needed on the class explicitly.

I don't think we are using it yet. I realize we will likely later, but I think we should wait to add dependencies until necessary.

I think bitstring works with any version of your code, it is the combination of bitstring + space_packet_parser that isn't working. I'm not sure how to explicitly call that out here though... Just indicating that I don't think it is ideal to pin bitstring back when it is really space_packet_parser that is causing the failures.

Indeed, I don't know what else to do though. If you do a regular poetry install without specifying bitstring version here, the code doesn't work. Gavin is working on a fix, we could just wait for that before merging.

Gavin already pushed a new version to pypi!

Can you add high level explanation of what this class is doing? It would be helpful.

It might be nice to add this in _parse_waveform_data function doc string because I was wondering what those (2, 4, 8) means.

What does scitype==1 means in concept?

There are different types of science packets we could expect, numbered [1, 2, 4, 8, 16, 32, 64].

"1" contains information about the events, and 2,3,8,16,32 and 64 contain the different 6 variables we're interested in. Maybe we can change these numbers to be more human-readable here.

I was wondering what is difference between epochs and data? This might relate to question I had about scitype == 1.

"epochs" is a list of times that the dust events have occurred, and the data is the actual data from those events. epoch is generally just the CDF/SPDFs term for "time".

can we spell out time_low_sr? more specifically, spell out sr

Sure thing, "sr" just stands for "sampling rate". 3 of the important variables use a very high sampling rate, and the 3 of the other ones use a (comparitavely) very low sampling rate.

Does evt stands for event?

Yes!

This isn't really a comment to this specific PR, but what are the chances of packet field names changing? If there's a possibility that they could change down the line would it make sense to create some type of adapter so we wouldn't need to go through and change these strings? Or if they did change would we not care since we can call them whatever we want in the xtce files?

So I'm certainly not an expert at decom or XTCE, but I think we're just assigning a human-readable name to those specific 8 bits in the file. I went through and changed the code and the packet definition xml file to call it 'IDX__SCI0TYPE_X' instead of "IDX__SCI0TYPE", and everything still decoded fine.

What will these Trigger variables be used for? In the case that some of these if conditions are not satisfied, they might not be defined when something tries to access them from the object, so should be initialized under init.

yeah these are only used for logging really, so I don't know if it matters that they are like this. I'm going to get rid of the "self." because I don't think the rest of the class needs to know or care about them.

It might be good to add a comment or assign those numbers (20, 10, -6) to variables, so it's clear where they're coming from.

I'm being nitpicky here, but could we use event_num and packet here instead of evtnum and pkt? I find that code is less readable when variables are unnecessarily abbreviated.

yeah for sure

It is not clear to me what ls stands for here

It is not clear to me what hs stands for here

high sampling, I'll spell it out in the next commit!

Following the examples from space_packet_parser: https://space-packet-parser.readthedocs.io/en/latest/users.html#basic-usage
where this will cleanup the files for you automatically.

Follow numpydoc conventions according to the style guide: https://imap-processing.readthedocs.io/en/latest/development/style-guide/api-documentation.html

Parameters
------------

There is also no filename parameter, and I think parameters can be documented within the constructor below and put attributes here.

You are within IDEX already throughout this module, so I'd leave it as xtce_file or something like that without the idex_ prefix everywhere.

not used anywhere?

Just set it at the end like you're already doing and then the types won't get conflicted either since it is an xarray dataset later on.

one-liner?

Do we always want these logs, or do we only want them while debugging?

logging.debug()

Add some warnings/raises if we are getting into the edge cases?

just remove the version dependency if you're going to put it at 0s?

or some more recent xarray is likely to be required.

I'm not sure how we feel about naming classes. I guess I'd vote for dropping IDEX personally. So that if I were to import it later it'd be: from imap_processing.idex import RawDustEvent and not duplicating IDEX. But, I don't have a strong preference.

what is hgdelay? can we spell out hg?