Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ensure multiple cases are put into a single cube #1050

Merged
merged 8 commits into from
Jan 22, 2025
Merged

Ensure multiple cases are put into a single cube #1050

merged 8 commits into from
Jan 22, 2025

Conversation

daflack
Copy link
Contributor

@daflack daflack commented Jan 17, 2025 •
edited
Loading

Ensure multiple cases are put into a single cube to allow aggregation across cases to be easily implemented.

Fixes #1048

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.
  • 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.

@daflack daflack self-assigned this Jan 17, 2025
@daflack daflack marked this pull request as draft January 17, 2025 10:32
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 17, 2025 •
edited
Loading

Coverage

@daflack daflack marked this pull request as ready for review January 17, 2025 14:45
@daflack daflack requested review from jwarner8 and jfrost-mo January 17, 2025 14:45
@daflack daflack force-pushed the 1048_aggregation_check branch from d1ff7bd to 6737e70 Compare January 20, 2025 16:55
@jfrost-mo jfrost-mo mentioned this pull request Jan 21, 2025
Merged
7 tasks
@daflack daflack force-pushed the 1048_aggregation_check branch 2 times, most recently from 9b54774 to a7c60f9 Compare January 22, 2025 11:44
@daflack daflack mentioned this pull request Jan 22, 2025
Merged
7 tasks
jfrost-mo
jfrost-mo reviewed Jan 22, 2025
View reviewed changes
Copy link
Member

@jfrost-mo 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.

Looking good. Just a small clarification needed around the CubeList aggregation code, then I'm happy to re-review and approve.

src/CSET/operators/_utils.py Outdated Show resolved Hide resolved
src/CSET/operators/_utils.py Outdated Show resolved Hide resolved
daflack
daflack commented Jan 22, 2025
View reviewed changes
src/CSET/operators/_utils.py Outdated Show resolved Hide resolved
Sylviabohnenstengel
Sylviabohnenstengel approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@Sylviabohnenstengel Sylviabohnenstengel left a comment

Choose a reason for hiding this comment

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

Happy with changes, but suggest that james takes a look at the tests.

src/CSET/operators/_utils.py Outdated Show resolved Hide resolved
src/CSET/operators/_utils.py Outdated Show resolved Hide resolved
src/CSET/operators/collapse.py Show resolved Hide resolved
src/CSET/operators/collapse.py Show resolved Hide resolved
@Sylviabohnenstengel
Copy link
Contributor

@daflack @jfrost-mo review done and suggest that James has a look at the tests to ensure everything is covered. especially around ensuring teh coordinates required.

jfrost-mo
jfrost-mo approved these changes Jan 22, 2025
View reviewed changes
Copy link
Member

@jfrost-mo jfrost-mo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me (including tests). I'll leave it to you to merge after you are done with Sylvia's comments.

jwarner8
jwarner8 approved these changes Jan 22, 2025
View reviewed changes
src/CSET/operators/_utils.py Outdated Show resolved Hide resolved
@daflack daflack force-pushed the 1048_aggregation_check branch from 28f7362 to 469cacf Compare January 22, 2025 16:12
@daflack daflack merged commit 1f1cb91 into main Jan 22, 2025
8 checks passed
@daflack daflack deleted the 1048_aggregation_check branch January 22, 2025 16:14
@jfrost-mo jfrost-mo removed the request for review from ukmo-huw-lewis January 23, 2025 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfrost-mo jfrost-mo jfrost-mo approved these changes

@Sylviabohnenstengel Sylviabohnenstengel Sylviabohnenstengel approved these changes

@jwarner8 jwarner8 jwarner8 approved these changes

Assignees

@daflack daflack

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create a single cube for aggregation across multiple cases
4 participants
@daflack @Sylviabohnenstengel @jwarner8 @jfrost-mo