fix: derive branded_variable when loading CMIP7 catalog from DB#712
Merged
Conversation
branded_variable (variable_id + "_" + branding_suffix) is computed at parse time but never stored as a DB column, so a catalog loaded from the database was missing it. The solver then raised KeyError: 'branded_variable' when applying CMIP7 data requirement filters (issue #687). Add an overridable _add_derived_columns() hook on DatasetAdapter, called in load_catalog (including the empty-catalog path), and override it in CMIP7DatasetAdapter to reconstruct branded_variable. _enrich_parsed_catalog now reuses the same hook. Strengthen round-trip coverage for both adapters: remove branded_variable from CMIP7 non_roundtrip_columns so test_round_trip verifies it survives the DB round-trip, and add test_derived_columns_survive_round_trip.
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Make derived (computed-on-load) columns a first-class adapter concept so the catalog round-trip contract is enforced in one place rather than duplicated in test config.
lewisjared
force-pushed
the
fix/cmip7-branded-variable
branch
from
9f4a4ea to
dfee0e3
Compare
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
1 participant
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.
Description
Fixes #687.
CMIP7's
branded_variable({variable_id}_{branding_suffix}) is computed at parse time but never stored as a database column. A catalog loaded from the database viaload_catalogwas therefore missing it, and the solver raisedKeyError: 'branded_variable'when applying CMIP7 data requirement filters.It cannot simply be added to
dataset_specific_metadata, because that tuple feeds the model constructor inregister_datasetandbranded_variableis a read-only hybrid property (no setter). Instead it is now reconstructed on load:_add_derived_columns()hook onDatasetAdapter, called inload_catalog(including the empty-catalog path). Base implementation is a no-op.CMIP7DatasetAdapteroverrides it to reconstructbranded_variable;_enrich_parsed_catalognow reuses the same hook so parse-time and load-time derivation stay identical.Why the tests missed it
The round-trip test existed and was parameterised over CMIP6/CMIP7, but CMIP7 listed
branded_variableinnon_roundtrip_columns, dropping it from the comparison. Other CMIP7 tests only exercisedfind_local_datasets(the raw catalog, which already adds the column), never the DB-load path the solver uses.Checklist
Please confirm that this pull request has done the following:
changelog/