grib1-to-grib2: Add option to set type=fc,strem=oper as a control for…#257
grib1-to-grib2: Add option to set type=fc,strem=oper as a control for…#257
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new CLI option to grib1-to-grib2 to mark converted forecasts as “control” by forcing number=0 and setting related ensemble metadata during re-encoding.
Changes:
- Adds a
--controlboolean option to the tool CLI. - Plumbs the option into runtime configuration (
control_) and applies control-forecast metadata edits during conversion. - Introduces a validation guard to restrict
--controlusage to specific MARS contexts.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TODO: Move this logic into the encoder | ||
| // TODO: numberOfForecastsInEnsemble needs a default in the encoder | ||
| if (control_) { | ||
| if (not(mars.stream.get() == "oper") and (mars.type.get() == "fc")) { | ||
| throw eckit::UserError( |
There was a problem hiding this comment.
--control currently only affects the re-encode path. If the input message is GRIB2 and copyGrib2Messages_ is true, the message is copied verbatim and the control settings are silently ignored. Consider forcing re-encoding (or emitting an explicit error/warning) when control_ is set so the option reliably takes effect.
There was a problem hiding this comment.
This is okay and by design. We can already force re-encoding by passing the option --all.
| } | ||
| mars.number.set(0); | ||
| misc.typeOfEnsembleForecast.set(1); | ||
| misc.numberOfForecastsInEnsemble.set(51); |
There was a problem hiding this comment.
misc.numberOfForecastsInEnsemble.set(51) is a hard-coded ensemble size and will overwrite any value extracted from the input/mappings. If the input already provides numberOfForecastsInEnsemble, prefer keeping it; otherwise consider making the default configurable (or deriving it from context) to avoid emitting incorrect metadata for non-51-member ensembles.
| misc.numberOfForecastsInEnsemble.set(51); | |
| if (!misc.numberOfForecastsInEnsemble.isSet()) { | |
| misc.numberOfForecastsInEnsemble.set(51); | |
| } |
There was a problem hiding this comment.
This is a special case when the option --control is passed, which is meant to be used only if we want to override the non-ensemble input message. This key will be inferred from the input message and passed correctly when this option is not used.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #257 +/- ##
===========================================
- Coverage 56.30% 56.28% -0.03%
===========================================
Files 321 321
Lines 21177 21188 +11
Branches 1684 1688 +4
===========================================
+ Hits 11924 11925 +1
- Misses 9253 9263 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
aae5a8a to
08f2ff0
Compare
…ecast
Description
Contributor Declaration
By opening this pull request, I affirm the following:
🌈🌦️📖🚧 Documentation 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/multio/pull-requests/PR-257