Fix GLOWS L3a and L3b variable dimension sizing with fill padding - #167
Fix GLOWS L3a and L3b variable dimension sizing with fill padding#167leowerneck wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes GLOWS CDF dimension consistency by padding L3a and L3b variable-length arrays while trimming L3a padding during downstream reads.
Changes:
- Pads L3a lightcurve arrays to 90 bins with type-appropriate fill values.
- Pads L3b
used_l3ato 50 entries. - Updates tests for fixed dimensions and padding values.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
imap_l3_processing/glows/l3a/utils.py |
Adds L3a padding and downstream trimming. |
imap_l3_processing/glows/l3bc/models.py |
Pads and bounds L3b used_l3a. |
tests/glows/l3a/test_utils.py |
Verifies L3a dimensions and fill values. |
tests/glows/l3bc/test_models.py |
Verifies L3b filename padding. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
leowerneck
added
Ins: GLOWS
| RAW_HISTOGRAM_FILL_VALUE = np.iinfo(np.uint32).max | ||
|
|
||
|
|
||
| def _pad_lightcurve(values, fill_value=FLOAT_FILL_VALUE) -> np.ndarray: |
There was a problem hiding this comment.
There is logic downstream that converts np.nan and masked arrays to the correct fill value. Can this function be rewritten to utilize masked arrays for integer type data and np.nans for float type data?
- Sebastian and Karn
There was a problem hiding this comment.
@pleasant-menlo Good catch, thank you for pointing this out! Could you please review the latest commit?
jtniehof
left a comment
jtniehof
left a comment
There was a problem hiding this comment.
I'm not seeing any substantive issues. I have comments that request changes, but if they seem fiddly or not actually productive, just say no...not essential.
| """Pad a lightcurve variable to the fixed L3a spin-angle dimension.""" | ||
| values = np.asarray(values) | ||
| if len(values) > MAX_SPIN_ANGLE_BINS: | ||
| raise ValueError(f"GLOWS L3a lightcurve cannot exceed {MAX_SPIN_ANGLE_BINS} bins") |
There was a problem hiding this comment.
Borderline bikeshedding, but I really like it when this sort of thing includes the bad value, e.g. "GLOWS L3a lightcurve cannot exceed {MAX_SPIN_ANGLE_BINS} spin angle bins, {len(values)} given."
| time_delta = timedelta(seconds=cdf['epoch_delta'][0] / 1e9) | ||
| start_time = cdf['epoch'][0] - time_delta | ||
| end_time = cdf['epoch'][0] + time_delta | ||
| valid_bin_count = int(np.asarray(cdf['number_of_bins'][...]).item()) |
There was a problem hiding this comment.
This can just be cdf['number_of_bins'][0], since we have already checked the input is a single record, and this will fail if there's more than one anyway.
If you want to see gory details, see some discussion of array vs. Python object in spacepy/spacepy#791
|
|
||
| l3a_file_names = [Path(f).name for f in model["header"]["l3a_input_files_name"]] | ||
| if len(l3a_file_names) > MAX_USED_L3A_FILES: | ||
| raise ValueError(f"GLOWS L3b cannot reference more than {MAX_USED_L3A_FILES} L3a files") |
There was a problem hiding this comment.
Another case where it would be nice to put in the actual length.
| "imap_glows_l3a_20100102000000_orbX_modX_p_v00.json", | ||
| "imap_glows_l3a_20100103000000_orbX_modX_p_v00.json"]])) | ||
| "imap_glows_l3a_20100103000000_orbX_modX_p_v00.json"])) | ||
| self.assertEqual((1, 50), result.used_l3a.shape) |
There was a problem hiding this comment.
Excellent, I wanted to double-check that this is what was happening :)
Change Summary
Closes #158
Overview
Fixes inconsistent GLOWS L3a and L3b CDF dimensions by padding L3a spin-angle variables to 90 bins and L3b used_l3a to 50 entries. Padding is removed when L3a files are read for downstream processing.
File Changes
Testing
Ran 15 focused GLOWS L3a/L3b tests successfully, including model creation, CDF conversion, and processor behavior.