Implement center/delta epoch values for most CoDICE L1a products#1906
Conversation
bourque
requested review from
greglucas,
joeymukherjee,
tech3371,
tmplummer and
vmartinez-cu
There was a problem hiding this comment.
Pull Request Overview
This PR adds centered epoch computation and corresponding delta-minus/plus variables to CoDICE L1a products, updates coordinate definitions and dataset creation routines, and introduces a new test for epoch values.
- Introduce
calculate_epoch_valueshelper to compute centered epoch and deltas. - Update
define_coordinates,create_direct_event_dataset, and related functions to includeepoch_delta_minusandepoch_delta_plus. - Add a parametrized test
test_l1a_validate_epoch_valuesand extend YAML configs with new variable attributes.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| imap_processing/tests/codice/test_codice_l1a.py | Add test for validating epoch values |
| imap_processing/codice/codice_l1a.py | Extract epoch/delta logic, update coordinate setup and dataset creation |
| imap_processing/cdf/config/imap_codice_l1b_variable_attrs.yaml | Add YAML entries for epoch_delta_minus/plus |
| imap_processing/cdf/config/imap_codice_l1a_variable_attrs.yaml | Add YAML entries for epoch_delta_minus/plus |
Comments suppressed due to low confidence (1)
imap_processing/tests/codice/test_codice_l1a.py:286
- The new test asserts only the centered epoch matches validation but doesn't cover
epoch_delta_minusorepoch_delta_plus. Add assertions for these two arrays to ensure full coverage of the new output variables.
np.testing.assert_allclose(
| dims=[name], | ||
| attrs=self.cdf_attrs.get_variable_attributes(name, check_schema=False), | ||
| ) | ||
| self.coords[name] = var |
There was a problem hiding this comment.
Here you're storing the raw ndarray instead of the DataArray you just created. It should be self.coords[name] = coord so downstream logic receives the DataArray.
| self.coords[name] = var | |
| self.coords[name] = coord |
There was a problem hiding this comment.
Great catch robot overlord!
| epoch = (acq_start[:-1] + acq_start[1:]) / 2 | ||
| epoch_delta_minus = epoch - acq_start[:-1] | ||
| epoch_delta_plus = acq_start[1:] - epoch | ||
|
|
||
| # The end values are calculated differently | ||
| epoch = np.concatenate([epoch, [acq_start[-1]]]) | ||
| epoch_delta_minus = np.concatenate([epoch_delta_minus, [epoch_delta_minus[-1]]]) | ||
| epoch_delta_plus = np.concatenate([epoch_delta_plus, [epoch_delta_plus[-1]]]) |
There was a problem hiding this comment.
This division produces floats but your function signature and attributes imply integer nanoseconds. Consider rounding or casting the result to an integer type to maintain consistency.
| epoch = (acq_start[:-1] + acq_start[1:]) / 2 | |
| epoch_delta_minus = epoch - acq_start[:-1] | |
| epoch_delta_plus = acq_start[1:] - epoch | |
| # The end values are calculated differently | |
| epoch = np.concatenate([epoch, [acq_start[-1]]]) | |
| epoch_delta_minus = np.concatenate([epoch_delta_minus, [epoch_delta_minus[-1]]]) | |
| epoch_delta_plus = np.concatenate([epoch_delta_plus, [epoch_delta_plus[-1]]]) | |
| epoch = np.round((acq_start[:-1] + acq_start[1:]) / 2).astype(int) | |
| epoch_delta_minus = np.round(epoch - acq_start[:-1]).astype(int) | |
| epoch_delta_plus = np.round(acq_start[1:] - epoch).astype(int) | |
| # The end values are calculated differently | |
| epoch = np.concatenate([epoch, [int(acq_start[-1])]]) | |
| epoch_delta_minus = np.concatenate([epoch_delta_minus, [int(epoch_delta_minus[-1])]]) | |
| epoch_delta_plus = np.concatenate([epoch_delta_plus, [int(epoch_delta_plus[-1])]]) |
There was a problem hiding this comment.
I think just adding .astype(int) to the end of line 700 fixes all of this. I think the other changes are not necessary?
| epoch_delta_minus = xr.DataArray( | ||
| epochs_delta_minus, | ||
| name="epoch_delta_minus", | ||
| dims=["epoch_delta_minus"], |
There was a problem hiding this comment.
The dimension name should match the coordinate dimension 'epoch', not its own name. Update to dims=["epoch"] so the deltas align with the epoch axis.
| self.plan_step = plan_step | ||
| self.view_id = view_id | ||
|
|
||
| def calculate_epoch_values(self) -> NDArray[int]: |
There was a problem hiding this comment.
I moved this method outside of the class and made it a function so I can re-use it in multiple places in the code.
| acq_start = met_to_ttj2000ns(acq_start_seconds + acq_start_subseconds / 1e6) | ||
|
|
||
| # Apply correction to center the epoch bin | ||
| epoch = ((acq_start[:-1] + acq_start[1:]) / 2).astype(int) |
There was a problem hiding this comment.
Another option is to use integer division.
| epoch = ((acq_start[:-1] + acq_start[1:]) / 2).astype(int) | |
| epoch = ((acq_start[:-1] + acq_start[1:]) // 2) |
| epoch_delta_minus = epoch - acq_start[:-1] | ||
| epoch_delta_plus = acq_start[1:] - epoch | ||
|
|
||
| # The end values are calculated differently |
There was a problem hiding this comment.
How? Could you add a comment here saying what you are doing. I would have naively assumed you would handle the minus and plus differently because they are going off different ends of the array. (one prepend, the other append)
This PR implements epoch values that are centered within the acquisition time window, as well as data variables for their corresponding
delta_minusanddelta_plusfor most CoDICE L1a data products. These calculations for thehskp,hi-omni, andhi-ialirtare a bit trickier and will be addressed in a separate PR.Another thing to note is that this PR will be the first of several that makes (relatively) small changes to CoDICE L1a data products. The goal is to eventually re-validate against updated validation data once all of those fixes are in. In the meantime, I may need to tweak or turn off certain unit tests to get tests passing in this middle-ground state between sets of validation data.
Partially completes #1501