Plume refactor for wind at height field computation by Plume#23
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #23 +/- ##
===========================================
+ Coverage 69.16% 72.55% +3.38%
===========================================
Files 78 88 +10
Lines 3162 3596 +434
Branches 273 313 +40
===========================================
+ Hits 2187 2609 +422
- Misses 975 987 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c4dc104 to
23c124a
Compare
135d5d0 to
e86c626
Compare
c8b116c to
05209dd
Compare
antons-it
left a comment
There was a problem hiding this comment.
This is a great addition to plume, thanks for this development! - it's going to be really useful to be able to handle derived fields directly in plume!! only minor suggestions/comments from me, but only for (potentially) future PR's
| ~ModelData(); | ||
|
|
||
| // --- creates (new values) | ||
| void createInt(std::string name, int valInit); |
There was a problem hiding this comment.
just wondering if it would not make sense to keep an explicit version of parameter creation methods beside the templated version? like createInt, createDouble, etc.., just for convenience?
There was a problem hiding this comment.
Summarising our conversation here too: I am reluctant to maintaining parallel functions for this as it would introduce a duplication in the API and end up increasing the maintenance cost instead of reducing it, as the current PR proposes with the createParam template. I would rather not create a legacy and have some plugins use the typed methods and other use the templates. Plugins running with a Fortran model remain limited in the types they can use because the C/Fortran APIs have not changed in this PR; and on the other hand, c++ only pipelines can immediately support any type of model data, including custom ones.
If we must keep the old ModelData API, then I'd rather have the createInt etc. be wrappers around the template call with a deprecation warning, than their original implementation. However, if the concern is about type restrictions, then I'd rather use type traits to enforce a specific usage of the template.
| - | ||
| - name: u | ||
| type: ATLAS_FIELD | ||
| height: 100 |
There was a problem hiding this comment.
I think this way of expressing that we are requesting a field that interpolates u at h=100m is nice and concise! and it's perfectly valid to "imply" which strategy to use from the presence of the "height" param. I'm wondering if for more advanced (future) strategies, it would also be nice to retain a "fully explicit" way of requesting a specific strategy (i.e. by name + strategy params)? - in any case, not something for this PR!
There was a problem hiding this comment.
I created the PLUME-61 ticket for this.
| EXPECT_NOT(u5000Ptr->isUpdated()); | ||
|
|
||
| EXPECT_THROWS(plume::field_provider::WindAtHeight invalidStrategy(80001, zPtr, uPtr, u5000Ptr)); | ||
| plume::field_provider::WindAtHeight u200Strategy(200, zPtr, uPtr, u200Ptr); |
There was a problem hiding this comment.
I think this is practical and perfectly valid to instantiate one strategy for each derived field - again, for the future (and if it turn out to be necessary!) we could think of instantiating a strategy first and then perhaps apply it to various fields? i.e. a smoothing or interpolation function that we might want to apply to several parameters?
There was a problem hiding this comment.
Summarising our conversation here: I chose the following ownership chain to keep dependencies between objects as clear and detangled as possible. If strategies were to be reused across parameters, then the ModelData would also need a share of the ownership at least during setup which would be entirely possible but I don't see the clear benefit for now, unless a strategy is very expensive and can be run only once to update all fields that depend on it (reminder: if 100u is requested multiple times, there will only be one instance in the modelData, so this would apply to cases where 100u & 200u etc. are requested). I think the current implementation is more maintainable, and allows the strategy implementation to remain confined to the scientific work as much as possible, which I think is a strength for potential scaling and expansion.
ModelData
├── [IParameterValue (shared)]
│ ├── IParameterObserver
│ │ └── UpdateStrategy (unique)
│ └── IParameterObservable
vs.
ModelData
├── [IParameterValue (shared)]
│ ├── IParameterObserver
│ │ └── UpdateStrategy (shared)
│ └── IParameterObservable
├── [UpdateStrategy (shared)]
Happy to make a ticket to keep the conversation on this part of the architecture going if you think it is necessary !
There was a problem hiding this comment.
Pull request overview
Refactors Plume’s data/parameter model to support derived diagnostics (notably wind-at-height) owned and recomputed by Plume via an observer/publisher pattern and pluggable update strategies, aligning with GRIB2-driven changes in model level handling.
Changes:
- Introduces typed
ParameterType/ParameterValueinfrastructure plus update-strategy registry and dependency wiring inModelData. - Adds
WindAtHeightstrategy and negotiation logic to request/activate derived parameters with dependencies. - Updates Manager/Protocol/plugins/tests/examples/C-API to use templated
require<T>,offer<T>, andgetParam<T>APIs.
Reviewed changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/nwp_emulator/nwp_emulator_plugin.h | Updates plugin negotiation to templated require<atlas::Field>(). |
| tests/nwp_emulator/nwp_emulator_plugin.cc | Updates data access to getParam<atlas::Field>(). |
| tests/nwp_emulator/data/plume_config_sp.yml | Renames simple plugin library to simple_plugins. |
| tests/nwp_emulator/data/plume_config_dp.yml | Renames simple plugin library to simple_plugins. |
| tests/nwp_emulator/CMakeLists.txt | Links tests against simple_plugins library. |
| tests/core/test_update_strategies.cc | Adds unit tests for update strategies (wind-at-height). |
| tests/core/test_protocol.cc | Adjusts protocol tests to set-based name comparisons. |
| tests/core/test_plugin_params.cc | Extends tests for derived params negotiation and invalid derived config. |
| tests/core/test_parameter.cc | Updates tests to ParameterDefinition (via ParameterCatalogue). |
| tests/core/test_model_data.cc | Adds/updates tests for typed params, ownership, updates, and observing params. |
| tests/core/test_manager_json.cc | Updates JSON manager tests and switches to set-based param names. |
| tests/core/test_manager.cc | Updates manager tests; adds coverage for derived-field activation and feed behavior. |
| tests/core/test_catalogue.cc | Updates to ParameterDefinition insertion. |
| tests/core/test_atlas_model_data.cc | Adds Atlas-focused ModelData tests including observation updates. |
| tests/core/simple_plugin.h | Updates plugin negotiation to templated require<int>(). |
| tests/core/simple_plugin.cc | Updates data access to getParam<int>(). |
| tests/core/simple_atlas_plugin.h | Adds a test plugin that requests a derived Atlas field (height option). |
| tests/core/simple_atlas_plugin.cc | Implements the derived-field test plugin core. |
| tests/core/ManagerTestAccess.h | Adds test-only access to Manager::reset(). |
| tests/core/CMakeLists.txt | Builds simple_plugins lib and adds new tests for atlas model data & strategies. |
| src/plume/data/README.md | Documents how to add/register new update strategies. |
| src/plume/data/ParameterValue.h | Introduces typed parameter wrapper + observable/observer roles. |
| src/plume/data/ParameterValue.cc | Implements observer/publisher wiring and naming helper. |
| src/plume/data/ParameterType.h | Adds compile-time type→enum mapping and string conversions. |
| src/plume/data/ParameterCatalogue.h | Replaces Parameter with ParameterDefinition, supports derivation metadata. |
| src/plume/data/ParameterCatalogue.cc | Implements derived-parameter detection, strategy matching, and set-based names. |
| src/plume/data/Parameter.h | Removes legacy Parameter and untyped ParameterValue hierarchy. |
| src/plume/data/Parameter.cc | Removes legacy Parameter implementation. |
| src/plume/data/ModelData.h | Refactors to typed param storage + strategy registry + dependency creation. |
| src/plume/data/ModelData.cc | Implements strategy registration, dependency wiring, and derived creation dispatch. |
| src/plume/data/FieldProvider.h | Adds UpdateStrategy framework + WindAtHeight trait/registry utilities. |
| src/plume/data/FieldProvider.cc | Implements strategy matching and WindAtHeight interpolation. |
| src/plume/api/plume.cc | Updates C API wrappers to templated Protocol/ModelData APIs. |
| src/plume/Protocol.h | Replaces per-type methods with templated require<T> / offer<T>. |
| src/plume/Protocol.cc | Updates implementation to ParameterDefinition and set-based name retrieval. |
| src/plume/PluginHandler.h | Stores PluginDecision instead of a flat list of param names. |
| src/plume/PluginHandler.cc | Exposes required/offered param names via PluginDecision. |
| src/plume/PluginDecision.h | Stores offered params as ParameterDefinition set; expands names with dependencies. |
| src/plume/PluginConfig.h | Adds missing exception include (build hygiene). |
| src/plume/Negotiator.h | Adds helper to validate offered/derived params. |
| src/plume/Negotiator.cc | Implements derived-param offering checks (source + dependencies). |
| src/plume/ManagerConfig.h | Adds missing exception include (build hygiene). |
| src/plume/Manager.h | Allows feedPlugins to mutate ModelData (create derived params); adds reset for tests. |
| src/plume/Manager.cc | Creates derived params on feed; skips derived in initial data checks; adds test reset. |
| src/plume/Configurable.h | Makes config_ protected for derived access. |
| src/plume/CMakeLists.txt | Adds new data sources (FieldProvider/ParameterType/ParameterValue) to build. |
| src/nwp_emulator/nwp_emulator.cc | Migrates emulator to templated Protocol/ModelData APIs. |
| src/nwp_emulator/README.md | Updates emulator invocation to `_sp |
| examples/plugin_foo.h | Updates example plugin negotiation to templated requires. |
| examples/plugin_foo.cc | Updates example plugin reads to getParam<int>(). |
| examples/plugin_bar.h | Updates example plugin negotiation to templated requires; adds Atlas include. |
| examples/plugin_bar.cc | Updates example plugin reads to getParam<...>(). |
| examples/example3.cc | Updates end-to-end example to templated offer/require and ModelData APIs. |
| examples/example2.cc | Updates example to templated offer/require and ModelData APIs. |
| examples/example1.cc | Updates example to templated offer/require and ModelData APIs. |
| cmake/plume-plugin-interface.cmake | Updates generated plugin code to use templated protocol.require<T>(). |
| README.md | Links “Plugin Data” section to src/plume/data/. |
| AUTHORS | Adds new contributors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. |
89d1316 to
5856aef
Compare
a6d5f5a to
a2294e9
Compare
7839b70 to
1fd55b2
Compare
with update strategies (WindAtHeight) + tests
…ger to handle derived parameters
1fd55b2 to
386ae73
Compare
antons-it
left a comment
There was a problem hiding this comment.
Great development!! - approved again after changes - thanks a lot for this!!
Description
This PR equips Plume with the ability to control its own share of the data, via an observer/publisher pattern and update strategies.
This need comes from the GRIB2 migration which impacts the height levels computed by IFS. Instead of deferring the wind at height computation to the plugin, Plume can manage this diagnostic field and serve it to the plugins as it would for any model field.
The strategy mechanism implemented is generic and allows extension to other data types beyond fields (see README in
plume/data/for steps to add new strategies). The only concrete strategy implemented computes the wind at a given height from a wind field and the geopotential, compare tovdfvintsubroutine for scientific correctness.To apply the strategies, Plume model data and parameter representations have been refactored to allow model variables to be observed by Plume-owned variables, such that updates trigger a recomputation of the observing parameters. A templated approach was followed to allow seamless extension to more data types and dependency types.
This PR is separated in two main commits, the first one introduces the model data and parameter refactor with the new patterns, and the second one focuses on the Plume Manager, and handling the change from the client side (requesting derived fields, negotiating them, and creating them).
Contributor Declaration
By opening this pull request, I affirm the following: