Updates to include missing ancillary file for glows #2630
Conversation
maxinelasp
requested review from
a team,
Copilot,
greglucas,
lacoak21 and
subagonsouth
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This pull request adds support for the missing l1b conversion table ancillary file for GLOWS processing. The conversion table is now loaded from a JSON file via the CLI and passed through the processing pipeline instead of being hardcoded within the processing functions.
Changes:
- Added CLI management for loading the l1b conversion table ancillary file and passing it to both histogram and direct event processing
- Updated function signatures throughout the codebase to pass conversion_table_dict/ancillary_parameters where needed
- Removed hardcoded file loading from DirectEventL1B.post_init and replaced it with a parameter-based approach
- Updated test fixtures and test function signatures to include mock_conversion_table_dict
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| imap_processing/cli.py | Loads conversion table JSON file and passes it to glows_l1b() and glows_l1b_de() functions |
| imap_processing/glows/l1b/glows_l1b.py | Updated function signatures to accept conversion_table_dict; created closure for passing ancillary_parameters in process_de() |
| imap_processing/glows/l1b/glows_l1b_data.py | Added ancillary_parameters as InitVar parameter to DirectEventL1B; removed hardcoded file loading; added physical_unit to validation |
| imap_processing/tests/glows/conftest.py | Added mock_conversion_table_dict fixture with physical_unit keys; updated fixtures to pass conversion table through |
| imap_processing/tests/glows/test_glows_l1b.py | Updated test function signatures to include mock_conversion_table_dict parameter |
| imap_processing/tests/glows/test_glows_l1b_data.py | Updated test function signatures to include mock_conversion_table_dict parameter |
| imap_processing/tests/glows/test_glows_l2.py | Updated test function signatures to include mock_conversion_table_dict parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| This class reads from either a dict (JSON input) or an xarray Dataset (from | ||
| GlowsAncillaryCombiner) which defines ancillary parameters. It validates to | ||
| ensure the input has all the required parameters. | ||
| Parameters | ||
| ---------- | ||
| input_table : dict | ||
| Dictionary generated from input JSON file. | ||
| input_table : dict or xr.Dataset | ||
| Dictionary generated from input JSON file, or xarray Dataset from | ||
| GlowsAncillaryCombiner containing conversion table data. |
There was a problem hiding this comment.
The class docstring states that input_table can be "dict or xr.Dataset", but the init method only accepts dict (line 261) and the implementation only handles dict operations (lines 285-300). Either the docstring should be updated to reflect that only dict is supported, or the implementation should be updated to handle xr.Dataset if that's a planned feature. Based on the current code usage where conversion_table_dict is loaded from JSON and passed directly, it appears only dict is currently supported.
| """Create a mock conversion table dataset for testing. | ||
| This fixture creates an xarray Dataset with the structure expected by | ||
| AncillaryParameters._extract_from_dataset(), with an epoch dimension | ||
| so it can be selected by day using .sel(epoch=day, method="nearest"). |
There was a problem hiding this comment.
The docstring is inaccurate and misleading. It states this fixture creates an "xarray Dataset" and mentions a non-existent method "AncillaryParameters._extract_from_dataset()". However, this fixture actually returns a plain dict, not an xarray Dataset, and is passed directly to AncillaryParameters.init(). The docstring should be updated to accurately describe that this fixture creates a dictionary with the structure expected by the AncillaryParameters constructor.
| """Create a mock conversion table dataset for testing. | |
| This fixture creates an xarray Dataset with the structure expected by | |
| AncillaryParameters._extract_from_dataset(), with an epoch dimension | |
| so it can be selected by day using .sel(epoch=day, method="nearest"). | |
| """Create a mock conversion table dictionary for testing. | |
| This fixture returns a plain dictionary with the structure and fields | |
| expected by the AncillaryParameters constructor. |
| Path(__file__).parents[1] / "ancillary" / "l1b_conversion_table_v001.json" | ||
| ) as f: | ||
| ancillary_parameters = AncillaryParameters(json.loads(f.read())) | ||
| # Extract conversion table for the current day |
There was a problem hiding this comment.
The comment "Extract conversion table for the current day" is misleading. Unlike the ancillary exclusions and pipeline settings which are filtered by day using .sel(epoch=day, method="nearest"), the conversion_table_dict is passed directly to AncillaryParameters without any day-based extraction. The comment should be updated to accurately reflect what the code does, such as "Create ancillary parameters from conversion table".
| # Extract conversion table for the current day | |
| # Create ancillary parameters from conversion table |
1 participant
Change Summary
During the ancillary file updates for GLOWS, I missed one. This PR will have a matching one to add the dependnecy in sds-data-manager. It primarily just wires up the file to the already-existing dataclass for passing the information around, so not a major change.
Overview
Testing