Filter set!(model, ::MetadataSet) by per-model accepted kwargs#274
Merged
Conversation
set!(model, mset) previously forwarded every glossary-mapped variable to set!(model; kwargs...), which fails for real Oceananigans/ClimaSeaIce models that throw ArgumentError on unknown kwargs. A shared 4-variable MetadataSet (T, S, h, ℵ) therefore broke both the ocean's set! (rejects h, ℵ) and the sea-ice set! (rejects T, S), as seen in the docs-build failure of examples/one_degree_simulation.jl. Add `accepted_metadata_names(model)` with model-specific methods for HydrostaticFreeSurfaceModel (introspect velocities/tracers/free_surface) and SeaIceModel (only :sea_ice_thickness, :sea_ice_concentration), and filter the routed kwargs through it. The fallback (`keys(variable_glossary)`) preserves existing StubModel test behavior. Add a RestrictiveStubModel regression test in test_metadata_set.jl that mimics the real models' kwarg-strictness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…dataSet) dispatch Previous commit introduced an `accepted_metadata_names` hook that the generic `set!(model, mset)` consulted to filter routed kwargs. Refactor to plain multiple dispatch: each model type overrides `set!(::Model, ::MetadataSet)` directly. The generic fallback delivers every glossary-mapped variable (useful for permissive stub models) and real models override to filter. Why this design is better: - Idiomatic Julia — no parallel hook layer, just multiple dispatch. - Per-model logic lives next to its dispatch, not behind a predicate. - Future models can do more than just filter (transform, validate, multi-pass) without having to extend a second function. A small `_set_glossary!` helper keeps the kwargs-construction code DRY across the dispatches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s arg Replace the private `_set_glossary!` helper with a third optional argument `names` on `Fields.set!(model, mset::MetadataSet, names=keys(variable_glossary))`. Per-model overrides simply compute their narrowed `names` and call the 3-arg generic — no separate helper, no redundant `haskey(variable_glossary, ...)` check (the default already restricts to glossary keys). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
|
Yeah! Thanks! I was about to open an issue! |
Member
Author
|
i want to merge bc this is urgent (0.5.0 is broken). can you approve? |
Member
Author
|
i'll merge because tests pass. |
Member
|
Hm... Did the test capture the case in the one_degree_model? set!(ocean.model, ecco_set) # picks up :temperature, :salinity → T, S
short = (propertynames(model.velocities)..., propertynames(model.tracers)..., propertynames(model.free_surface)...) = (:u, :v, :w, :T, :S, :e, :displacement, :barotropic_velocities, :filtered_state, :gravitational_acceleration, :kernel_parameters, :substepping, :timestepper)
names = (:eastward_velocity, :v_velocity, :temperature, :u_velocity, :eastward_wind, :northward_wind, :air_temperature, :northward_velocity, :salinity)
[ Info: Downloading ECCO data: temperature in /home/constantinon/.julia/scratchspaces/904d977b-046a-4731-8b86-9235c0d1ef02/ECCO/v4...
[ Info: ... downloaded 274.0 B (100% complete, 552.080 ms)
[ Info: ... downloaded 274.0 B (100% complete, 552.161 ms)
[ Info: ... downloaded 274.0 B (100% complete, 552.193 ms)
[ Info: ... downloaded 274.0 B (100% complete, 552.210 ms)
ERROR: RequestError: HTTP/2 403 while requesting https://ecco.jpl.nasa.gov/drive/files/Version4/Release4/interp_monthly/THETA/1993/THETA_1993_01.ncSeems that |
Member
I was trying to test! |
Member
|
I guess the docs built on |
Member
Author
|
ah sorry! ~~yes, it looks like the MetadataSet for ECCO is including velocities, although we may not want to set them. ~~ I don't think it does: NumericalEarth.jl/examples/one_degree_simulation.jl Lines 86 to 88 in 05507ee |
Member
|
What could be a clean solution for that? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
examples/one_degree_simulation.jl:set!(ocean.model, ecco_set)andset!(sea_ice.model, ecco_set)both threwArgumentError: name h not found in model.velocities, model.tracers, or model.free_surfacebecause the genericset!(model, ::MetadataSet)forwarded every glossary-mapped variable to the model, but realHydrostaticFreeSurfaceModel/SeaIceModelthrow on unknown kwargs.set!(::HydrostaticFreeSurfaceModel, ::MetadataSet)andset!(::SeaIceModel, ::MetadataSet)that filter the routed kwargs to those the model actually consumes.set!(model, ::MetadataSet)fallback simple: deliver every glossary-mapped variable. Permissive stub/ad-hoc models continue to work; strict models override to filter.Design
Plain multiple dispatch — no
accepted_metadata_nameshook in between. Per-model logic lives next to its dispatch, and a future model can do more than just filter (transform, validate, multi-pass) without having to extend a second function. A small_set_glossary!helper keeps the kwargs-construction code DRY across the two dispatches.Why this got through CI
The existing
set!(model, mset)tests intest/test_metadata_set.jluse a permissiveStubModelwhoseset!accepts arbitrary kwargs, so the dispatch's failure to filter wasn't exercised. Docs build did fail on the #259 merge commit but the run shows ascancelled, so it wasn't a hard block.Test plan
julia --project=. test/test_metadata_set.jlpasses locally, including a newRestrictiveStubModelregression test that mimics the real models' kwarg-strictness with a shared 4-variable MetadataSet and demonstrates the override pattern.which(set!, (HydrostaticFreeSurfaceModel, MetadataSet))andwhich(set!, (SeaIceModel, MetadataSet))route to the specialised methods, not the fallback.🤖 Generated with Claude Code