Skip to content

Add complete OMNI variable support#83

Merged
sahiljhawar merged 9 commits into
GFZ:mainfrom
Simon060899:codex/omni-all-variables
Jul 19, 2026
Merged

Add complete OMNI variable support#83
sahiljhawar merged 9 commits into
GFZ:mainfrom
Simon060899:codex/omni-all-variables

Conversation

@Simon060899

@Simon060899 Simon060899 commented Jul 17, 2026

Copy link
Copy Markdown

Summary

  • add registry-driven access to all 42 one-minute and 45 five-minute OMNIWeb variables
  • add all 54 non-time OMNI2 fields, including support for historic 55-word and current 57-word hourly records
  • preserve the existing nine high-resolution and three hourly default columns and their ordering
  • add variables=None, variables="all", ordered subset selection, normalized aliases, and available_variables() metadata discovery
  • store complete cadence-valid processed schemas and upgrade partial legacy caches only when expanded reads require it
  • preserve UTC indices, source-file provenance, atomic writes, parallel monthly downloads, and focused-reader return schemas

Compatibility and robustness

Existing calls retain their previous schemas because the new argument is optional. The implementation also adds explicit validation for cadence-incompatible selections, empty or invalid selections, mixed timezone inputs, invalid ranges, malformed NASA responses, invalid time fields, corrupt caches, and unsupported hourly record widths.

Focused Dst, Kp, F10.7, and SYM-H readers delegate normalized time and range handling to their parent OMNI readers. Their established output windows and schemas remain unchanged.

Cache replacement remains atomic and uses pathlib and tempfile, so the implementation is portable across Linux, macOS, and Windows-style path handling.

Documentation

  • add a dedicated OMNI user-guide page with complete variable tables, NASA identifiers, units, fill values, cadence restrictions, API examples, cache behavior, compatibility guarantees, error guidance, and source acknowledgements
  • expand public docstrings and focused implementation comments
  • link the new guide from the README and Sphinx index

Verification

  • deterministic OMNI suite: 124 passed, 2 opt-in live checks skipped
  • opt-in NASA smoke tests: 2 passed, covering all 42 one-minute and all 45 five-minute fields
  • production-size live checks: complete one- and five-minute months, plus a 2024 hourly 55-word file expanded to all 54 output fields
  • complete local tests/io suite: 588 passed; 10 existing SIDC tests whose data source is unavailable and 2 opt-in NASA checks skipped
  • both opt-in NASA checks were run separately and passed, so only the 10 unavailable SIDC tests remain unexecuted
  • Ruff, formatting, byte-compilation, and Sphinx documentation builds passed
  • REUSE compliance passed

The manually dispatched Linux/macOS Python 3.11–3.14 matrix established that every OMNI test passes on every requested combination. Seven jobs failed only the existing live SuperMAG test (zero or one of two requested days returned); macOS/Python 3.11 received both responses and passed completely. The Ty job reports the same two RBMDataSet diagnostics reproduced on untouched upstream/main; neither is in a file changed by this PR.

Reviewer entry points

  • swvo/io/omni/variables.py — authoritative variable metadata and selection rules
  • swvo/io/omni/omni_high_res.py — complete OMNIWeb request, parsing, and monthly cache-upgrade flow
  • swvo/io/omni/omni_low_res.py — version-tolerant OMNI2 parsing and yearly cache-upgrade flow
  • docs/omni_variables.rst — complete public guide

@sahiljhawar

Copy link
Copy Markdown
Collaborator

@Simon060899 in child classes which are inheriting from OMNI, I think tz enforcement and time inequality assertion can be removed since parent class already have them.

A similar refactoring should be performed for DSTOMNI to make things consistent.

let me know if you need help

@sahiljhawar

Copy link
Copy Markdown
Collaborator

@Simon060899

Copy link
Copy Markdown
Author

Thanks for the suggestion. This is addressed in d0f2ae0.

I removed the duplicate timezone enforcement and time-range validation from F107OMNI, KpOMNI, and SymhOMNI, so their parent OMNI readers now own that validation.

I also refactored DSTOMNI to delegate to OMNILowRes.read(..., variables="dst") and removed its duplicate file-reading implementation. The focused hourly readers still require their existing boundary padding, so that policy now lives in the parent reader through a protected subclass setting. This preserves the existing Dst, Kp, and F10.7 output windows while keeping normalized time handling centralized.

I added regression coverage for parent delegation, mixed naive/UTC inputs, invalid ranges, cadence boundaries, and provenance. All deterministic tests passed. The 2 opt-in NASA smoke checks also passed; only 10 existing SIDC tests remain skipped because their data source is unavailable.

@Simon060899

Copy link
Copy Markdown
Author

Agreed—removed in 95d88f2, together with its Sphinx index entry. The user-facing OMNI guide remains, while the implementation and verification details are retained in the PR description.

@sahiljhawar

Copy link
Copy Markdown
Collaborator

@Simon060899

Copy link
Copy Markdown
Author

Thanks. I added SPDX-FileContributor: Simon Mischel to all 17 files changed by this PR in 542b18ae. REUSE compliance, Ruff, formatting, the strict OMNI documentation build, and the complete tests/io suite pass (588 passed, 12 skipped).

Regarding the linked review comments, the link currently opens the Files changed view but does not display an inline conversation for me. If the review is still pending, could you please submit it or repost the inline comments? I’ll address them promptly.

@sahiljhawar

Copy link
Copy Markdown
Collaborator

Okay, thanks.
The review comment is

  1. I would prefer methods to be instance method, an option would be to define the available_variables as a utility function which takes cadence as inputand resolves it into omni low res (cadence = None) or omni high res (cadence = 1 or 5), this method then can be used as instance method (called on the instance i.e swvo.io.omni.OMNIHighRes("./data").available_variables(cadence=1|5) or swvo.io.omni.OMNILowRes("./data").available_variables()) and standalone function (swvo.io.omni.variables.available_variables(cadence=None|1|5)).

  2. since this is already a private method (static methods starting with _), it would make sense to be a instance method.

  3. For both omni low res and high res.

@Simon060899

Copy link
Copy Markdown
Author

Thanks, addressed in bde01dda.

I added the shared cadence-aware swvo.io.omni.variables.available_variables(cadence=None|1|5) utility. OMNIHighRes.available_variables(cadence=...) and OMNILowRes.available_variables() are now instance methods that delegate to it.

I also converted the private cadence and cache helpers to instance methods in both readers and updated the documentation and regression tests.

@sahiljhawar

Copy link
Copy Markdown
Collaborator

LGTM. I will merge it. Thanks for the effort.

@sahiljhawar
sahiljhawar merged commit f794b9e into GFZ:main Jul 19, 2026
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants