Adding RBMNcDataSet#17
Conversation
|
LGTM! |
There was a problem hiding this comment.
Pull Request Overview
Adds NetCDF support for RBM datasets and modernizes type hints across IO functions.
- Introduces RBMNcDataSet with NetCDF reading, variable mapping, and tests.
- Refactors enums and type aliases (InstrumentLike, MfmLike) and adjusts MAT file naming fields.
- Replaces list[...] with Sequence[...] in multiple IO readers; minor test updates and utility fixes.
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/io/RBMDataSet/test_utils.py | Adjusts test to create a file without extension, aligning with glob logic. |
| tests/io/RBMDataSet/test_RBMNcDataset.py | New tests covering RBMNcDataSet behavior and variable computations. |
| tests/io/RBMDataSet/test_RBMDataset.py | Type annotations and expected path updates for existing RBMDataSet tests. |
| swvo/io/solar_wind/read_solar_wind_from_multiple_models.py | Changes model_order type to Sequence. |
| swvo/io/kp/read_kp_from_multiple_models.py | Changes model_order and rec_model_order to Sequence; exposes KpModel in init.py. |
| swvo/io/kp/init.py | Re-exports KpModel. |
| swvo/io/hp/read_hp_from_multiple_models.py | Changes model_order to Sequence and docstring adjustments. |
| swvo/io/f10_7/read_f107_from_multiple_models.py | Changes model_order to Sequence. |
| swvo/io/dst/read_dst_from_multiple_models.py | Changes model_order to Sequence. |
| swvo/io/RBMDataSet/utils.py | Tightens extension selection logic and warnings. |
| swvo/io/RBMDataSet/custom_enums.py | Adds StrEnum-based MfmEnum, InstrumentLike/MfmLike aliases; renames Variable fields; adds instrument. |
| swvo/io/RBMDataSet/init.py | Exposes RBMNcDataSet and new type aliases. |
| swvo/io/RBMDataSet/RBMNcDataSet.py | New NetCDF-backed dataset implementation and helpers. |
| swvo/io/RBMDataSet/RBMDataSet.py | Accepts InstrumentLike/MfmLike; adjusts MAT filename logic; equality and dir improvements. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if mfm.upper() in MfmEnum.__members__: | ||
| mfm = MfmEnum[mfm.upper()] |
There was a problem hiding this comment.
Case handling for MfmEnum is incorrect and can leave self._mfm as a str (e.g., 'T04s' becomes 'T04S' and misses members), which later breaks when accessing .value. Handle strings explicitly and avoid uppercasing for mixed-case names. Example: if isinstance(mfm, str): key = mfm if mfm in MfmEnum.members else mfm.upper(); if key in MfmEnum.members: mfm = MfmEnum[key]; elif not isinstance(mfm, MfmEnum): raise ValueError('Invalid mfm'); then assign self._mfm = mfm.
| if mfm.upper() in MfmEnum.__members__: | |
| mfm = MfmEnum[mfm.upper()] | |
| if isinstance(mfm, str): | |
| key = mfm if mfm in MfmEnum.__members__ else mfm.upper() | |
| if key in MfmEnum.__members__: | |
| mfm = MfmEnum[key] | |
| else: | |
| raise ValueError(f"Invalid mfm: {mfm}") | |
| elif not isinstance(mfm, MfmEnum): | |
| raise ValueError(f"Invalid mfm: {mfm}") |
There was a problem hiding this comment.
this review sort of make sense. I tried it locally and if the mfm is a invalid string, it is still set to self._mfm. And even if the mfm is a valid str, value self._mfm is set to a str mfm istead of the corresponding MfmEnum, which I think is wrong as it will break the self._mfm.value
There was a problem hiding this comment.
this also fails for mfm: str = "T04s"
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.