-
Notifications
You must be signed in to change notification settings - Fork 1
Feed model_id and variant_label to recipe
#309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
mo-nikosbaltas
merged 33 commits into
main
from
287-feed-model_id-and-variant_label-to-recipe
Jan 8, 2026
Merged
Changes from all commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
80f05a4
Config changes for adding a second model dev run
mo-nikosbaltas 6cf6121
Reformatted rose-*.conf filesafter CI failed
mo-nikosbaltas dd6140b
changes related to #286 and standardise model data
mo-nikosbaltas 4ed330c
#286 after merging with #285 and main
mo-nikosbaltas 23f5754
added CDDS/standardisation support for two models
mo-nikosbaltas 3200651
Update CMEW/flow.cylc
mo-nikosbaltas 4900f99
Update CMEW/app/standardise_model_data/rose-app.conf
mo-nikosbaltas 52fe013
Update CMEW/app/configure_standardise/bin/configure_standardise.sh
mo-nikosbaltas 2988ecb
changes implemented as suggested in PR 305 review
mo-nikosbaltas 1fd6a96
used the env variable VARIANT_LABEL
mo-nikosbaltas cc0ab8e
Merge branch 'main' into 287-feed-model_id-and-variant_label-to-recipe
mo-nikosbaltas 71beadb
changes to support dual model runs with recipes containing models_id …
mo-nikosbaltas 7fbc3d9
ensures two datasets entries are present
mo-nikosbaltas 7d0d99b
Update CMEW/app/configure_for/bin/test_update_recipe_file.py
mo-nikosbaltas b492145
Update CMEW/app/configure_for/bin/test_update_recipe_file.py
mo-nikosbaltas adc40d1
Update CMEW/app/configure_for/bin/test_update_recipe_file.py
mo-nikosbaltas 96cc418
added/edited comments suggested
mo-nikosbaltas d83d0cb
Merge branch '287-feed-model_id-and-variant_label-to-recipe' of githu…
mo-nikosbaltas 2067f71
comments refinement
mo-nikosbaltas 704cdb7
documentation for dual runs
mo-nikosbaltas eec1dee
Revert "documentation for dual runs"
mo-nikosbaltas a040aaf
updated for dual-model run
mo-nikosbaltas cc0ee6f
Merge branch 'main' into 287-feed-model_id-and-variant_label-to-recipe
alistairsellar aecaeaf
Update doc/source/user_guide/workflow.rst
mo-nikosbaltas 26bd40e
Update doc/source/user_guide/workflow.rst
mo-nikosbaltas 13327eb
Update doc/source/user_guide/workflow.rst
mo-nikosbaltas 8cf02ca
Update doc/source/user_guide/workflow.rst
mo-nikosbaltas 1f5076e
Update doc/source/user_guide/workflow.rst
mo-nikosbaltas 6c37de2
Update doc/source/user_guide/workflow.rst
mo-nikosbaltas 745927e
Update doc/source/user_guide/workflow.rst
mo-nikosbaltas 9e423bc
some typo corrections
mo-nikosbaltas 8a1d710
chenged some confusing comments
mo-nikosbaltas 160f614
edited comments
mo-nikosbaltas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, I think we're changing this from "historical". Is it deliberate? If so, the comment should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, I should have picked this up in first review. Actually I think that experiment might be wrong for both runs, including the original. For this recipe (radiation budget) the choice of experiment doesn't make a difference, but it will for some recipes, so
experimentshould be something that the user defines as part of the model run definition. I've just opened an issue to add that: #316.For this PR, I propose that we accept that the second run is no more wrong than the first, and that having them consistently called "amip" is as good as any choice. I.e. I propose that we keep
"exp": "amip"for both.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think the comment should reflect what's going on, but I will take note to pay more attention to the unchanged code in a review next time.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had spent some time debugging the failure last week when implementing the #287 and I dug out the logs I had kept, so here is the explanation for completion.
There was a failure because the reference dataset was treated as a CMIP6 “historical” run in the recipe, while CDDS had standardised it as a GCModelDev / ESMVal / amip run.
From the ESMValTool log for the reference dataset (dataset index 0, HadGEM3):
'dataset': 'HadGEM3-GC31-LL',
'project': 'ESMVal',
'mip': 'Amon',
'short_name': 'hfls',
'activity': 'CMIP',
'alias': 'None',
'ensemble': 'r5i1p3f3',
'exp': 'historical',
...
So, after executing update_recipe_file.py:
• ‘project’ has been changed to ESMVal
• ‘ensemble’ is r5i1p1f3 (from REF_VARIANT_LABEL)
• But:
o ‘exp’ was still historical
o ‘activity’ was still CMIP
Now looking at where ESMValTool is searching for files (in the logs):
Looked for files matching
/home/users/nikolaos.baltas/cylc-run/CMEW_287/test287c/share/work/GCModelDev/CMIP/MOHC/HadGEM3-GC31-LL/historical/r5i1p1f3/Amon/hfls/gn//hfls_Amon_HadGEM3-GC31-LL_historical_r5i1p1f3_gn.nc
/home/users/nikolaos.baltas/cylc-run/CMEW_287/test287c/share/work/GCModelDev/CMIP/NERC/HadGEM3-GC31-LL/historical/r5i1p1f3/Amon/hfls/gn//hfls_Amon_HadGEM3-GC31-LL_historical_r5i1p1f3_gn.nc
Key bits:
• Path includes GCModelDev/CMIP/.../historical/r5i1p1f3/...
• This is driven by ‘activity: CMIP’ and ‘exp: historical’.
However, the CDDS request (from create_request_file.py) uses:
"experiment_id": "amip",
and is run twice (REF and EVAL) via standardise_model_data. So CDDS is standardising:
• GCModelDev/ESMVal//amip//...
for both runs.
That means:
• CDDS has produced ‘amip’ data
• ESMValTool is still looking for ‘historical’ data for the reference dataset
• Hence: “No input files found for Dataset ... historical ...”
The evaluation dataset works fine because we had explicitly set:
'project': 'ESMVal',
'activity': 'ESMVal',
'exp': 'amip',
'ensemble': 'r1i1p1f1',
...
and ESMValTool finds (from logs):
Found input files for Dataset: hfls, Amon, ESMVal, UKESM1-0-LL, ESMVal, amip, r1i1p1f1, gn, v20251230
So, the fix was to make the reference dataset use the GCModelDev/ESMVal “amip” semantics too.
Need to also override ‘exp’ and ‘activity’ for the ‘reference dataset’ in the same way we do for the evaluation dataset.
I updated that block to:
# Reference dataset: treat as a GCModelDev / ESMVal / amip run,
ref_dataset = datasets[0]
ref_dataset.update(
{
"dataset": ref_model_id,
"project": "ESMVal",
"exp": "amip",
"activity": "ESMVal",
"ensemble": ref_variant,
"start_year": start_year,
"end_year": end_year,
}
)
# Evaluation dataset: ESMVal / amip run using MODEL_ID + VARIANT_LABEL
eval_dataset = datasets[1]
eval_dataset.update(
{
"dataset": eval_model_id,
"project": "ESMVal",
"exp": "amip",
"activity": "ESMVal",
"ensemble": eval_variant,
"start_year": start_year,
"end_year": end_year,
}
)
That aligns both datasets with:
• project: ESMVal
• activity: ESMVal
• exp: amip
which matches what CDDS is actually producing from create_request_file.py.
I hope this answers the question of why overriding 'project' and 'exp' . Now if this is the correct approach we need to investigate further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy that this comment is resolved from my perspective, but will leave open for @mo-nikosbaltas to close if he and @alistairsellar are satisfied that the "investigate further" aspect has been / is elsewhere addressed.