Update IDEX #695

anamanica · 2024-07-17T20:04:48Z

Change Summary

Overview

~~Generate an updated packet definition file with recent version of space_packet_parser~~
- This has been placed on a bit of a hold as we don't have a packet definition file for IDEX.
Separate out the L0 decommutation code and the L1 processing code within idex_packet_parser.py, and update unit tests accordingly.
- Separated out the code to mirror the format of other instruments. I kept most of the tests the same. I decided to keep in the CDF tests in there because they did help Bryan, Matthew and I catch some bugs--but I can delete these if that is preferred.
- I was going to expand upon the l1 tests, but the final test test_idex_tof_high_data_from_cdf covers a good portion of this. I would just be testing different variables that wouldn't be verified from the IDEX team. Again, I can expand upon this if that would be preferred.
Switch CDF attribute definitions to use yaml files instead of dataclasses
- I created the appropriate .yaml files, and switched to using ImapCdfAttributes

New Dependencies

None

New Files

idex_l1.py
- Holds processing code for l1
idex_l0.py
- Holds processing code for l0
imap_idex_global_cdf_attrs.yaml
- Holds global cdf attrs
imap_idex_l1_variable_attrs.yaml
- Holds variable cdf attrs

Deleted Files

idex_packet_parser
- This code is now split into idex_l1.py and decom_idex.py
idex_cdf_attrs
- This code is now held in Imap_idex_global_cdf_atrs.yaml and imap_idex_l1_variable_attrs.yaml

Updated Files

test_l1_cdfs.py
- Updated and worked with tests
- Changed name to test_idex_l1
test_decom.py
- Changed name to test_idex_l0.py

Testing

idex_l0.py and idex_l1.py

closes #656

bourque · 2024-07-18T16:31:47Z

Here is the error in the doc build: https://github.com/IMAP-Science-Operations-Center/imap_processing/actions/runs/9995062714/job/27626330356?pr=695#step:5:233

I think this might be failing because you will need to add __init__.py files to the l0/ and l1/ subdirectories in order to make those part of the 'package' (and thus importable).

anamanica · 2024-07-18T17:04:35Z

@bourque I just added those init.py files, but still no dice. Do I need to put anything in them? I see that most other init files are empty, but some have version.

bourque

I still need to finish reviewing idex_l1, but I thought I would send off what I have reviewed so far in the meantime.

Thanks for making these updates! Everything is looking good so far!

bourque · 2024-07-18T16:46:23Z

docs/source/code-documentation/idex.rst

@@ -15,4 +15,5 @@ Level 1 Processing Code:
    :template: autosummary.rst
    :recursive:

-    idex_packet_parser
+    l0.decom_idex


Suggested change

l0.decom_idex

l0.idex_l0

I think this will also solve the doc build issue

bourque · 2024-07-18T16:48:02Z

imap_processing/tests/idex/test_idex_l0.py



 @pytest.fixture(scope="session")
 def decom_test_data():
-    test_file = Path(
+    test_file = (


Is there a reason for this change here?

I think that might have been a mistake on my part when I was trying to get the tests to work. I just added it back in, everything is working fine!

bourque · 2024-07-18T17:01:29Z

imap_processing/cdf/config/imap_idex_l1_variable_attrs.yaml

+int_fillval: &int_fillval -9223372036854775808
+min_epoch: &min_epoch -315575942816000000
+max_epoch: &max_epoch 3155630469184000000
+
+data_min: &data_min 0
+data_max: &data_max 4096
+
+sample_rate_min: &sample_rate_min -130
+sample_rate_max: &sample_rate_max 130
+
+int_maxval: &int_maxval 9223372036854775807 # np.iinfo(np.int64).max


I hadn't realized you can define commonly-used values like this. I am going to steal this for CoDICE!

bourque · 2024-07-18T17:02:32Z

imap_processing/cdf/config/imap_idex_l1_variable_attrs.yaml

+data_min: &data_min 0
+data_max: &data_max 4096


I don't see these being used anywhere else in the file. Could they be removed?

Just double checked idex_cdf_attributes.py from dev branch, and these are supposed to be used for l1_data_base. My bad!

bourque · 2024-07-18T17:04:13Z

imap_processing/cdf/config/imap_idex_l1_variable_attrs.yaml

+int_maxval: &int_maxval 9223372036854775807 # np.iinfo(np.int64).max
+
+# l1_data_base
+l1_data_base: &l1_data_base # ScienceAttrs


Suggested change

l1_data_base: &l1_data_base # ScienceAttrs

l1_data_base: &l1_data_base

We can probably remove references to the old dataclasses since we will no longer be using them

bourque · 2024-07-18T17:08:12Z

imap_processing/cdf/config/imap_idex_l1_variable_attrs.yaml

+  FORMAT: I12
+  UNITS: ""
+
+# TODO: All of these have var_notes in idex_cef_attrs. Should I add these?


Looks like VAR_NOTES is an optional attribute in terms of ISTP compliance, and just offers additional information. If it is not too much trouble to add them, I think it is worth doing.

And it looks like a few other instruments use them (hi and swe)

bourque · 2024-07-18T17:12:16Z

imap_processing/cdf/config/imap_idex_l1_variable_attrs.yaml

+  FIELDNAM: TOF High Double Pulse Max Samples
+  CATDESC: Maximum number of samples between pulse 1 and 2 for TOF. High double pulse triggering.
+  LABLAXIS: Samples Number
+  UNITS: "" # "samples" in idex_packet_parser...?


I think samples is fine in these? Perhaps counts would also work? This really is a question for the IDEX instrument team in what they want to see, I think. @bryan-harter do you have an idea?

bourque · 2024-07-18T17:14:20Z

imap_processing/idex/l0/idex_l0.py

Nicely done

bourque · 2024-07-18T17:19:13Z

imap_processing/idex/l1/idex_l1.py

-This module contains code to decommutate IDEX packets and creates xarrays to
-support creation of L1 data products.
-"""
+"""Decommutate IDEX CCSDS packets."""


This module now does something different. I would suggest saying something like what I have for CoDICE:

Perform CoDICE l1a processing. This module processes decommutated CoDICE packets and creates L1a data products.

bourque · 2024-07-18T17:20:06Z

imap_processing/idex/l1/idex_l1.py

-import dataclasses
 import logging
 from collections import namedtuple
 from enum import IntEnum

 import numpy as np
 import space_packet_parser
 import xarray as xr
-from space_packet_parser import parser, xtcedef

-from imap_processing import imap_module_directory
-from imap_processing.cdf.global_attrs import ConstantCoordinates
+from imap_processing.cdf.imap_cdf_manager import ImapCdfAttributes
 from imap_processing.cdf.utils import met_to_j2000ns
-from imap_processing.idex import idex_cdf_attrs
+from imap_processing.idex.l0.idex_l0 import decom_packets


There is a warm fuzzy feeling that I get when I see the removal/simplification of dependencies 😂

bourque

Nice work here, this looks good to me! I have a few small suggestions, but nothing blocking.

bourque · 2024-07-18T18:02:41Z

imap_processing/idex/l1/idex_l1.py

+        TriggerDescription(
+            "low_sample_coincidence_mode_blocks", "IDX__TXHDRLSTRIGCMBLOCKS"
+        ),


You could probably throw a # noqa at the end of this line so that ruff doesn't auto-correct this. I think its fine that this line exceeds the line limit for the sake of being consistent with the other lines.

Ah! That would be great! For some reason, # noqa is not working here... Is there something else I could try?

I was playing around with this and its a bit finicky, but I got this to work. Needed to add #fmt: skip as well, I guess to tell ruff-format check to ignore the line specifically

trigger_description_dict = { trigger.name: trigger for trigger in [ TriggerDescription("event_number", "IDX__TXHDREVTNUM"), TriggerDescription("tof_high_trigger_level", "IDX__TXHDRHGTRIGLVL"), TriggerDescription("tof_high_trigger_num_max_1_2", "IDX__TXHDRHGTRIGNMAX12"), TriggerDescription("tof_high_trigger_num_min_1_2", "IDX__TXHDRHGTRIGNMIN12"), TriggerDescription("tof_high_trigger_num_min_1", "IDX__TXHDRHGTRIGNMIN1"), TriggerDescription("tof_high_trigger_num_max_1", "IDX__TXHDRHGTRIGNMAX1"), TriggerDescription("tof_high_trigger_num_min_2", "IDX__TXHDRHGTRIGNMIN2"), TriggerDescription("tof_high_trigger_num_max_2", "IDX__TXHDRHGTRIGNMAX2"), TriggerDescription("tof_low_trigger_level", "IDX__TXHDRLGTRIGLVL"), TriggerDescription("tof_low_trigger_num_max_1_2", "IDX__TXHDRLGTRIGNMAX12"), TriggerDescription("tof_low_trigger_num_min_1_2", "IDX__TXHDRLGTRIGNMIN12"), TriggerDescription("tof_low_trigger_num_min_1", "IDX__TXHDRLGTRIGNMIN1"), TriggerDescription("tof_low_trigger_num_max_1", "IDX__TXHDRLGTRIGNMAX1"), TriggerDescription("tof_low_trigger_num_min_2", "IDX__TXHDRLGTRIGNMIN2"), TriggerDescription("tof_low_trigger_num_max_2", "IDX__TXHDRLGTRIGNMAX2"), TriggerDescription("tof_mid_trigger_level", "IDX__TXHDRMGTRIGLVL"), TriggerDescription("tof_mid_trigger_num_max_1_2", "IDX__TXHDRMGTRIGNMAX12"), TriggerDescription("tof_mid_trigger_num_min_1_2", "IDX__TXHDRMGTRIGNMIN12"), TriggerDescription("tof_mid_trigger_num_min_1", "IDX__TXHDRMGTRIGNMIN1"), TriggerDescription("tof_mid_trigger_num_max_1", "IDX__TXHDRMGTRIGNMAX1"), TriggerDescription("tof_mid_trigger_num_min_2", "IDX__TXHDRMGTRIGNMIN2"), TriggerDescription("tof_mid_trigger_num_max_2", "IDX__TXHDRMGTRIGNMAX2"), TriggerDescription("low_sample_coincidence_mode_blocks", "IDX__TXHDRLSTRIGCMBLOCKS"), # noqa TriggerDescription("low_sample_trigger_polarity", "IDX__TXHDRLSTRIGPOL"), TriggerDescription("low_sample_trigger_level", "IDX__TXHDRLSTRIGLVL"), TriggerDescription("low_sample_trigger_num_min", "IDX__TXHDRLSTRIGNMIN"), TriggerDescription("low_sample_trigger_mode", "IDX__TXHDRLSTRIGMODE"), TriggerDescription("tof_low_trigger_mode", "IDX__TXHDRLSTRIGMODE"), TriggerDescription("tof_mid_trigger_mode", "IDX__TXHDRMGTRIGMODE"), TriggerDescription("tof_high_trigger_mode", "IDX__TXHDRHGTRIGMODE"), TriggerDescription("detector_voltage", "IDX__TXHDRHVPSHKCH0"), TriggerDescription("sensor_voltage", "IDX__TXHDRHVPSHKCH1"), TriggerDescription("target_voltage", "IDX__TXHDRHVPSHKCH2"), TriggerDescription("reflectron_voltage", "IDX__TXHDRHVPSHKCH3"), TriggerDescription("rejection_voltage", "IDX__TXHDRHVPSHKCH4"), TriggerDescription("detector_current", "IDX__TXHDRHVPSHKCH5"), ] } # fmt: skip

bourque · 2024-07-18T18:04:14Z

imap_processing/idex/l1/idex_l1.py

+    return idex_attrs
+
+
+# Pretty sure this should go in idex_l1.py


Suggested change

# Pretty sure this should go in idex_l1.py

Yes!

bourque · 2024-07-18T18:07:56Z

imap_processing/idex/l1/idex_l1.py

    ]
 }


+def create_idex_attr_obj(data_version: str) -> ImapCdfAttributes:


Personal preference and nitpicky here, but I would prefer this be named get_idex_attrs -- the way it is currently named reads to me like some sort of class is being created that will later be modified. Might just be a me problem though 😂, it's totally up to you.

bourque · 2024-07-18T18:08:53Z

imap_processing/idex/l1/idex_l1.py


+        # TODO: Turn decom_packet_list back into generator type?


My opinion here is that I don't think that is necessary

bourque · 2024-07-18T18:12:07Z

imap_processing/idex/l1/idex_l1.py

+    # TODO: Do I need to add the data_version here,
+    #  or is that something I can get from the header_packet?


I think the way you have it now is the right way to go -- ultimately we just need the data_version to be part of the global attributes so that it is defined in the final dataset.

tech3371

can we run files produced from this through SKEditor for checks? I am unsure about the dimensions and coordinates. Thank you!

In general, really nice refactor and very impressive work!

tech3371 · 2024-07-19T21:48:02Z

imap_processing/cdf/config/imap_idex_global_cdf_attrs.yaml

+  <<: *instrument_base
+  Data_level: L2
+  Data_type: L2_SCI>Level-2 Science Data
+  Logical_source: idex_l2_global_attrs


wanted to mention that this will fail with filename convention. It should be something like this

Suggested change

Logical_source: idex_l2_global_attrs

Logical_source: imap_idex_l2_sci

tech3371 · 2024-07-19T21:54:27Z

imap_processing/idex/l1/idex_l1.py

            )

        # Process the 6 primary data variables
        tof_high_xr = xr.DataArray(
            name="TOF_High",
            data=[self._parse_high_sample_waveform(self.TOF_High_bits)],
-            dims=("epoch", "Time_High_SR_dim"),
-            attrs=idex_cdf_attrs.tof_high_attrs.output(),
+            dims=("epoch", "time_high_ssr_dim"),


I think there is a type here

Suggested change

dims=("epoch", "time_high_ssr_dim"),

dims=("epoch", "time_high_sr_dim"),

tech3371 · 2024-07-19T21:55:36Z

imap_processing/idex/l1/idex_l1.py

        )
        tof_low_xr = xr.DataArray(
            name="TOF_Low",
            data=[self._parse_high_sample_waveform(self.TOF_Low_bits)],
-            dims=("epoch", "Time_High_SR_dim"),
-            attrs=idex_cdf_attrs.tof_low_attrs.output(),
+            dims=("epoch", "time_high_sr"),


should this be this?

Suggested change

dims=("epoch", "time_high_sr"),

dims=("epoch", "time_high_sr_dim"),

see my other comment :)

tech3371 · 2024-07-19T21:59:40Z

imap_processing/idex/l1/idex_l1.py

            data=[self._calc_high_sample_resolution(len(tof_low_xr[0]))],
-            dims=("epoch", "Time_High_SR_dim"),
-            attrs=idex_cdf_attrs.high_sr_attrs.output(),
+            dims=("epoch", "time_high_sr_dim"),


As I am looking through these dims=(....), I am curious how it passed creating CDF or tests. I thought we need to add all these dimension to xr.Dataset's coords. I see some of those but not some.

And seems like we have removed some _dim but not this. remove? Also, do you know what dim stands for?

anamanica added 11 commits July 16, 2024 11:21

This branch is better.

e20b2dc

Trying to resolve issue

71885c9

I DID IT

01b4b2f

Separated l0 and l1

bf20368

Deleted files

9f07bde

Fixing tests

40a5c14

Attempting to fix docs

ac25fe0

Fixing

874868b

Docs check failing

c5aee06

Small change

73aec6e

Cleaning up

fd72e3f

anamanica changed the title ~~Update IDEX~~ [WIP] Update IDEX Jul 18, 2024

anamanica marked this pull request as ready for review July 18, 2024 16:26

anamanica requested review from bourque and bryan-harter July 18, 2024 16:26

Fixing Docs Error

3df1b01

bourque reviewed Jul 18, 2024

View reviewed changes

Fixing some PR comments

e89ee3e

bourque approved these changes Jul 18, 2024

View reviewed changes

bourque assigned anamanica Jul 18, 2024

Fixing format issues

694516b

anamanica merged commit ac3bc02 into IMAP-Science-Operations-Center:dev Jul 19, 2024
17 checks passed

anamanica deleted the testing-update-idex branch July 19, 2024 14:54

anamanica changed the title ~~[WIP] Update IDEX~~ Update IDEX Jul 19, 2024

tech3371 reviewed Jul 19, 2024

View reviewed changes

greglucas added the Ins: IDEX Related to the IDEX instrument label Aug 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update IDEX #695

Update IDEX #695

anamanica commented Jul 17, 2024 •

edited

Loading

bourque commented Jul 18, 2024

anamanica commented Jul 18, 2024

bourque left a comment

bourque Jul 18, 2024

bourque Jul 18, 2024

anamanica Jul 18, 2024

bourque Jul 18, 2024

bourque Jul 18, 2024

anamanica Jul 18, 2024

bourque Jul 18, 2024

bourque Jul 18, 2024

bourque Jul 18, 2024

bourque Jul 18, 2024

bourque Jul 18, 2024

bourque Jul 18, 2024

bourque Jul 18, 2024

bourque left a comment

bourque Jul 18, 2024

anamanica Jul 18, 2024

bourque Jul 18, 2024

bourque Jul 18, 2024

bourque Jul 18, 2024

bourque Jul 18, 2024

bourque Jul 18, 2024

tech3371 left a comment

tech3371 Jul 19, 2024

tech3371 Jul 19, 2024

tech3371 Jul 19, 2024

tech3371 Jul 19, 2024

tech3371 Jul 19, 2024

tech3371 Jul 19, 2024

	l1_data_base: &l1_data_base # ScienceAttrs
	l1_data_base: &l1_data_base

		# TODO: Do I need to add the data_version here,
		# or is that something I can get from the header_packet?

	Logical_source: idex_l2_global_attrs
	Logical_source: imap_idex_l2_sci

	dims=("epoch", "time_high_ssr_dim"),
	dims=("epoch", "time_high_sr_dim"),

	dims=("epoch", "time_high_sr"),
	dims=("epoch", "time_high_sr_dim"),

Update IDEX #695

Update IDEX #695

Conversation

anamanica commented Jul 17, 2024 • edited Loading

Change Summary

Overview

New Dependencies

New Files

Deleted Files

Updated Files

Testing

bourque commented Jul 18, 2024

anamanica commented Jul 18, 2024

bourque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bourque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech3371 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anamanica commented Jul 17, 2024 •

edited

Loading