Skip to content

Updates recipes to add in extra constraining for speed#1986

Merged
James Frost (jfrost-mo) merged 1 commit intomainfrom
1981_fix_constraining_in_masked_timeseries_recipes
Mar 31, 2026
Merged

Updates recipes to add in extra constraining for speed#1986
James Frost (jfrost-mo) merged 1 commit intomainfrom
1981_fix_constraining_in_masked_timeseries_recipes

Conversation

@daflack
Copy link
Copy Markdown
Collaborator

@daflack David Flack (daflack) commented Mar 25, 2026

Updates the recipes as suggested by Scott Wales (@ScottWales) in a generic form to help speed up the land sea mask recipes.

Fixes #1981

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.

@github-actions
Copy link
Copy Markdown
Contributor

Coverage

@daflack
Copy link
Copy Markdown
Collaborator Author

This is ready to review, but am keeping it as draft to ensure it is not merged prior to further discussion with James Frost (@jfrost-mo). It maybe that some of the PRs (e.g. #1905) in development may supersede this PR, and so we will not need to merge it.

@daflack David Flack (daflack) self-assigned this Mar 25, 2026
@daflack
Copy link
Copy Markdown
Collaborator Author

Note: if the route applied here is the decided route we will need to update all other recipes where we derive diagnostics to be similar.

@ukmo-huw-lewis
Copy link
Copy Markdown
Contributor

See #1999 for deployment of this approach successfully.

Note needed to also run with a cell_methods constraint here to avoid loading in multiple time-processed instances of same variable. Not 100% clear on equivalence to ['VARNAME', land_binary_mask] here (i.e. does applying empty cell_methods to land_binary_mask work ok? Was the original land_sea_mask recipes tested on inputs with potential for multiple instances of same varname?

  - operator: filters.filter_multiple_cubes
    constraint:
      operator: constraints.generate_cell_methods_constraint
      cell_methods: []
      varname: ['$VARNAME_BASE', '$VARNAME_OVER', '$VARNAME_CONTOUR']

@jfrost-mo James Frost (jfrost-mo) marked this pull request as ready for review March 31, 2026 08:10
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.

This extra constraining seems like a sensible approach to reduce the number of cubes loaded. My only concern is whether the name "land_binary_mask" is sufficiently generic to be used across different models, but given it was already hard-coded it should be fine.

When I get #1905 to completion it will remove most of the benefit of this change, but it still won't hurt, so I see no reason not to do it.

@jfrost-mo James Frost (jfrost-mo) merged commit f9b352e into main Mar 31, 2026
8 checks passed
@jfrost-mo James Frost (jfrost-mo) deleted the 1981_fix_constraining_in_masked_timeseries_recipes branch March 31, 2026 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unconstrained loading in masked time series

4 participants