Skip to content

Reorder difference so CSET does $OTHER - $BASE#2047

Merged
James Frost (jfrost-mo) merged 4 commits into
mainfrom
2046_diffswap
May 5, 2026
Merged

Reorder difference so CSET does $OTHER - $BASE#2047
James Frost (jfrost-mo) merged 4 commits into
mainfrom
2046_diffswap

Conversation

@jwarner8
Copy link
Copy Markdown
Contributor

Differences should be test minus control, this PR addresses #2046

Contribution checklist

Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.

  • Documentation has been updated to reflect change.
  • New code has tests, and affected old tests have been updated.
  • All tests and CI checks pass.
  • Ensured the pull request title is descriptive.
  • Ensure rose-suite.conf.example has been updated if new diagnostic added.
  • Conda lock files have been updated if dependencies have changed.
  • Attributed any Generative AI, such as GitHub Copilot, used in this PR.
  • Marked the PR as ready to review.

@jwarner8 James Warner (jwarner8) added the cleanup Non-functional improvement label Apr 13, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 13, 2026

Coverage

@jwarner8
Copy link
Copy Markdown
Contributor Author

Handing over to David Flack (@daflack) to determine what needs doing to SSIM before review

@daflack
Copy link
Copy Markdown
Collaborator

David Flack (daflack) commented Apr 14, 2026

Notes for SSIM:

  • switch other and base order in SSIM (both spatial and mean)
  • switch order in recipes
  • switch reference values to use base rather than other

@jwarner8
Copy link
Copy Markdown
Contributor Author

TODO check for impacts on cset_comparison_base through code

@ukmo-huw-lewis
Copy link
Copy Markdown
Contributor

For discussion - propose there may be value in a further re-design here to remove explicit reference to "BASE_MODEL" and "OTHER_MODEL" in the *_difference recipe. I wonder if some of this originated from difference plots pre-dating CSET functionality to work with multiple model inputs (e.g. for overplotting N models in timeseries, histograms etc).

At present, if looking to run surface_spatial_difference recipe for 2 models, I provide input variables as follows.....
cset bake -r ${path_to_recipes}/recsurface_fields/surface_spatial_difference.yaml --input-dir "${input_dir}" "${input_dir2}" --output-dir ${output_dir} ... --BASE_MODEL="${model_name}" --OTHER_MODEL="${model_name2}"
(i.e. the 2 input model paths are caught in 1 definition of input-dir but I separate out naming of BASE and OTHER.

For other recipes with multiple model inputs (e.g. time series), we now call with something like....
cset bake -r ${path_to_recipes}/surface_fields/generic_surface_domain_mean_time_series.yaml --input-dir "${input_dir}" "${input_dir2}" --output-dir ${output_dir} ... --MODEL_NAME="['${model_name}', '${model_name2}']"
(i.e. MODEL_NAME is a list of inputs).

I would propose adapting difference recipes to take a single MODEL_NAME input variable consistent with other recipes, and then adopt protocol of all differences as model[N] - model[0] in the list. This means e.g. analysis can be defined as first model in list and will always be identified as the 'control'.

Argument against this is that spatial_difference recipe only expects 2 model inputs (i.e. does not actually loop over N models). Is the ability to do this (remove looping over BASE/OTHER combinations in workflow) desirable? For command-line use, I could envisage benefit for users to "give me differences of my N models relative to 0" for a list of models > 2.

Happy if this gets pushed to separate issue, but hope useful to flag here if this PR starts to touch on relevant parts of differencing infrastructure.

@jfrost-mo
Copy link
Copy Markdown
Member

James Frost (jfrost-mo) commented Apr 15, 2026 via email

@jfrost-mo
Copy link
Copy Markdown
Member

Anke F (@afinnen) this is the reordering of the difference plots pull request.

@jfrost-mo
Copy link
Copy Markdown
Member

#2061 has been opened to track Huw's comment.

@jwarner8
Copy link
Copy Markdown
Contributor Author

Can I check the latest on this branch? It looks like SSIM has been completed. Frost opened an issue to track Huws comment. Is this ready for review?

@jfrost-mo
Copy link
Copy Markdown
Member

With SSIM completed I'm happy to review this.

Copy link
Copy Markdown
Member

@jfrost-mo James Frost (jfrost-mo) left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. We'll need to make sure we prominently include this in the release notes of our next release, but the updated recipes should help with that.

Grepping through the recipes I can't spot any missed reversals of the difference order, and the actual code change is simple enough.

@jfrost-mo James Frost (jfrost-mo) merged commit 8361d10 into main May 5, 2026
7 checks passed
@jfrost-mo James Frost (jfrost-mo) deleted the 2046_diffswap branch May 5, 2026 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup Non-functional improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants