Repository navigation
Add generic Norm object for oops - #203
DrTVockerodtMO wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The registered norm cannot be instantiated or used because every operation throws unconditionally, and it lacks automated coverage.
1 open finding
What changed in this PR
Adds the ORCA model-specific Norm interface required by the dependent OOPS change.
Changes:
- Registers
NorminOrcaModelTraits. - Adds the
Normclass and method stubs. - Includes the new sources in the build.
| File | Description |
|---|---|
src/orca-jedi/utilities/OrcaModelTraits.h |
Exposes the ORCA norm type. |
src/orca-jedi/norm/Norm.h |
Declares the norm interface. |
src/orca-jedi/norm/Norm.cc |
Defines currently unimplemented operations. |
src/orca-jedi/CMakeLists.txt |
Adds norm sources to the library. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Norm::Norm(const oops::Variables &, | ||
| const eckit::Configuration &) { | ||
| throw eckit::NotImplemented(Here()); |
There was a problem hiding this comment.
Silly copilot that is literally the point of the change is to safely abort when the unimplemented norm is called!
|
I think copilot is wrong here, but it does raise the question, when combined with the upstream OOPS change would we expect our 3DVar tests to start failing? |
Toby Searle (twsearle)
left a comment
Toby Searle (twsearle)
left a comment
There was a problem hiding this comment.
I added a comment above, but just to elaborate, orca-jedi CI doesn't have build-group syntax implemented yet. Have you ran our tests in a build including the upstream OOPS change?
I don't think so no, because currently the |
Yes, I ran this with mo-bundle on cylc as it was necessary to get everything to compile. It seems that the orca-jedi tests have passed: https://cylchub/services/cylc-review/taskjobs/terence.vockerodt?&suite=mobbm-xyz-jc_term_generic_norm_2%2Frun1&no_fuzzy_time=0&tasks=ctest_orca_jedi__exab_gnu |
|
N.B. because there are a lot of model interface changes required we may investigate a solution which doesn't require this change, I will keep you posted if/when that happens! |
OK! I will stick it in draft for now so I don't merge by accident. |
Description
Adds required interface
Normobject for https://github.com/JCSDA-internal/oops/pull/3413.Dependencies
List the other PRs that this PR is dependent on: