864 hi l1b compute coincidence type and time deltas #995

subagonsouth · 2024-10-08T20:22:58Z

Change Summary

Overview

Adds the Hi L1B algorithm for computing coincidence type and delta_t values. These fields share enough commonality in computation that I decided to implement them in a single function.

Updated Files

imap_processing/hi/l1b/hi_l1b.py
- Added function for computing L1B coincidence_type and delta_t variables
- Coincidence type and delta_t variables are now added to the dataset within that function.
imap_processing/hi/utils.py
- Added hi constants frozen dataclass similar to how other instruments have implemented
imap_processing/tests/hi/test_hi_l1b.py
- Added fixture that generates synthetic L1A data for testing coincidence_type and delta_t algorithm
- Added test coverage for compute_coincidence_type_and_time_deltas function

Closes: #864

…r direct events

sdhoyt

Looks great! Just hoping to get some clarification on how to read those tables in the comments

sdhoyt · 2024-10-09T17:18:31Z

imap_processing/hi/l1b/hi_l1b.py

+    # |      2      |      B      |      A      |      C1     |      C2     |
+    # |      3      |      C1     |      A      |      B      |      C2     |
+    # Set coincidence type bitmask
+    out_ds.coincidence_type[a_mask | tof_1_valid_mask] |= CoincidenceBitmap.A.value


I think because you're using an IntEnum you can do CoincidenceBitmap.A instead of CoincidenceBitmap.A.value

imap_processing/hi/l1b/hi_l1b.py

sdhoyt · 2024-10-09T17:27:48Z

imap_processing/tests/hi/test_hi_l1b.py

+def synthetic_trigger_id_and_tof_data():
+    """Create synthetic minimum dataset for testing the
+    coincidence_type_and_time_deltas algorithm."""
+    # The following coincidence type table shows possible values to consider


This table is really helpful! I should add something similar for Lo's coincidence types

sdhoyt · 2024-10-09T17:29:28Z

imap_processing/tests/hi/test_hi_l1b.py

+    tof2s = np.array(np.concatenate((HiConstants.TOF2_BAD_VALUES, [2])))
+    tof3s = np.array(np.concatenate((HiConstants.TOF3_BAD_VALUES, [3])))
+    var_names = ["trigger_id", "tof_1", "tof_2", "tof_3"]
+    data = np.meshgrid(ids, tof1s, tof2s, tof3s)


Great idea to more easily cover the combinations!

I can't quite take credit... found it on SO.

Simplify C1 coincidence type mask logic Remove .value from IntEnums where not needed

laspsandoval

I think that programmatically everything looks in order. My suggestion would be to add more docstrings, but you can take it or leave it.

docs/source/conf.py

imap_processing/tests/hi/test_hi_l1b.py

imap_processing/hi/l1b/hi_l1b.py

laspsandoval · 2024-10-11T14:56:35Z

imap_processing/hi/l1b/hi_l1b.py

+        -out_ds.tof_1.values[b_mask & tof_1_valid_mask].astype(np.float32)
+        * HiConstants.TOF1_TICK_PER_NS
+    )
+    out_ds.delta_t_ab.values[c_mask & tof_1and2_valid_mask] = (


I think that this could be split into two separate steps for clarity:

delta_t_2 = out_ds.tof_2.values[c_mask & tof_1and2_valid_mask].astype(np.float32) * HiConstants.TOF2_TICK_PER_NS
delta_t_1 = out_ds.tof_1.values[c_mask & tof_1and2_valid_mask].astype(np.float32) * HiConstants.TOF1_TICK_PER_NS
out_ds.delta_t_ab.values[c_mask & tof_1and2_valid_mask] = delta_t_2 - delta_t_1

laspsandoval · 2024-10-11T14:57:23Z

imap_processing/hi/l1b/hi_l1b.py

+        out_ds.tof_2.values[a_mask & tof_2_valid_mask].astype(np.float32)
+        * HiConstants.TOF2_TICK_PER_NS
+    )
+    out_ds.delta_t_ac1.values[b_mask & tof_1and2_valid_mask] = (


I think this could be split for clarity:

delta_t_2 = out_ds.tof_2.values[b_mask & tof_1and2_valid_mask] * HiConstants.TOF2_TICK_PER_NS
delta_t_1 = out_ds.tof_1.values[b_mask & tof_1and2_valid_mask] * HiConstants.TOF1_TICK_PER_NS
out_ds.delta_t_ac1.values[b_mask & tof_1and2_valid_mask] = delta_t_2 - delta_t_1

laspsandoval · 2024-10-11T14:58:23Z

imap_processing/hi/l1b/hi_l1b.py

+    )
+
+    # delta_t_bc1
+    out_ds.delta_t_bc1.values[a_mask & tof_1_valid_mask & tof_2_valid_mask] = (


Same here I would split it up:
delta_t_2 = out_ds.tof_2.values[a_mask & tof_1and2_valid_mask] * HiConstants.TOF2_TICK_PER_NS
delta_t_1 = out_ds.tof_1.values[a_mask & tof_1and2_valid_mask] * HiConstants.TOF1_TICK_PER_NS
out_ds.delta_t_bc1.values[a_mask & tof_1_valid_mask & tof_2_valid_mask] = delta_t_2 - delta_t_1

laspsandoval · 2024-10-11T15:00:25Z

imap_processing/hi/l1b/hi_l1b.py

+    # |      2      |      B      |     A,B     |     B,C1    |    C1,C2    |
+    # |      3      |      C1     |     A,C1    |     B,C1    |    C1,C2    |
+    # Set coincidence type bitmask
+    out_ds.coincidence_type[a_mask | tof_1_valid_mask] |= CoincidenceBitmap.A


For me personally I would really appreciate a docstring on each bitmask. Maybe it's overkill for some people, but it helps me to immediately understand the purpose.

Example:
Set the 'A' bit in the 'coincidence_type' variable where:

The trigger is 'A' (using a_mask), OR

tof_1 is valid (using tof_1_valid_mask).

My philosophy is that if you need a bunch of comments to explain your code, then the code is not written in a way that it can be clearly understood. IMO, the comments you wrote are exactly what the code says with a slightly different order. For this instance, I think the comment is redundant.

Rework some of the code to make it more readable

subagonsouth · 2024-10-11T20:46:17Z

I think that programmatically everything looks in order. My suggestion would be to add more ~~docstrings~~ comments, but you can take it or leave it.

@laspsandoval, I took your feedback about needing more comments to mean that my code was not easily understandable. I made some minor tweaks that make it much more readable, IMO. I also added some comments and think that it should be very clear now.

sdhoyt

Thanks for the walkthrough of all this. I think everything looks good!

subagonsouth added Ins: Hi Related to the IMAP-Hi instrument Level: L1 Level 1 processing labels Oct 8, 2024

subagonsouth added this to the Oct 2024 milestone Oct 8, 2024

subagonsouth requested review from laspsandoval and a team October 8, 2024 20:22

subagonsouth self-assigned this Oct 8, 2024

subagonsouth requested review from bourque, sdhoyt and tech3371 and removed request for a team October 8, 2024 20:23

subagonsouth added 4 commits October 8, 2024 14:26

Add hi_l1b function for computing coincidence type and time deltas fo…

8f575a1

…r direct events

Add test for compute coincidence type and time deltas funcion

92e6bcc

Add test coverage for coincidence type and delta T computation

aa8d841

Cleanup for PR

87215f8

subagonsouth force-pushed the 864-hi-l1b-compute-coincidence_type-and-time-deltas branch from a7c06ec to 87215f8 Compare October 8, 2024 20:26

Fix broken doc build

c240a7b

subagonsouth removed the request for review from bourque October 9, 2024 15:20

sdhoyt reviewed Oct 9, 2024

View reviewed changes

Clearify coincidence type table

0cae67f

Simplify C1 coincidence type mask logic Remove .value from IntEnums where not needed

laspsandoval approved these changes Oct 11, 2024

View reviewed changes

Address PR comments

5a9fea3

Rework some of the code to make it more readable

sdhoyt approved these changes Oct 14, 2024

View reviewed changes

subagonsouth merged commit cbb9132 into IMAP-Science-Operations-Center:dev Oct 14, 2024
17 checks passed

subagonsouth deleted the 864-hi-l1b-compute-coincidence_type-and-time-deltas branch October 14, 2024 17:58

tech3371 removed this from the Oct 2024 milestone Oct 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

864 hi l1b compute coincidence type and time deltas #995

864 hi l1b compute coincidence type and time deltas #995

subagonsouth commented Oct 8, 2024

sdhoyt left a comment

sdhoyt Oct 9, 2024

sdhoyt Oct 9, 2024

sdhoyt Oct 9, 2024

subagonsouth Oct 9, 2024

laspsandoval left a comment

laspsandoval Oct 11, 2024

laspsandoval Oct 11, 2024

laspsandoval Oct 11, 2024

laspsandoval Oct 11, 2024

subagonsouth Oct 11, 2024

subagonsouth commented Oct 11, 2024

sdhoyt left a comment

864 hi l1b compute coincidence type and time deltas #995

864 hi l1b compute coincidence type and time deltas #995

Conversation

subagonsouth commented Oct 8, 2024

Change Summary

Overview

Updated Files

sdhoyt 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

laspsandoval 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

subagonsouth commented Oct 11, 2024

sdhoyt left a comment

Choose a reason for hiding this comment