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.

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

Add mask operators #967

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Add mask operators #967

wants to merge 14 commits into from

Conversation

daflack
Copy link
Contributor

@daflack daflack commented Dec 5, 2024

Adds a generate and apply mask operator.

Fixes #221

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 added the enhancement New feature or request label Dec 5, 2024
@daflack daflack self-assigned this Dec 5, 2024
@daflack daflack marked this pull request as draft December 5, 2024 13:49
Copy link
Contributor

github-actions bot commented Dec 5, 2024

Coverage

@daflack
Copy link
Contributor Author

daflack commented Dec 9, 2024

Three example recipes provided, workflow changes are not part of this pull request and may link to the need to add custom recipes to the workflow.

@daflack daflack marked this pull request as ready for review December 9, 2024 15:06
@daflack daflack requested review from jwarner8 and jfrost-mo December 9, 2024 15:06
Copy link
Contributor

@jwarner8 jwarner8 left a comment

Choose a reason for hiding this comment

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

Just started reviewing this, looks like a great addition. Thanks for developing this. Think I am a bit unclear about apply_mask and how it interacts with multiple cubes/masks. Once I understand will complete review

src/CSET/operators/filters.py Show resolved Hide resolved
src/CSET/operators/filters.py Show resolved Hide resolved
src/CSET/operators/filters.py Outdated Show resolved Hide resolved
src/CSET/operators/filters.py Outdated Show resolved Hide resolved
src/CSET/recipes/simple_mask.yaml Outdated Show resolved Hide resolved
@daflack
Copy link
Contributor Author

daflack commented Dec 12, 2024

Review comments completed @jfrost-mo and @jwarner8 back to you.

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.

Overall looking like a good change. I've left some code comments, but nothing there is particularly fundimental.

I haven't looked at the tests yet; will do that later.

src/CSET/recipes/combined_mask_addition.yaml Outdated Show resolved Hide resolved
src/CSET/operators/filters.py Outdated Show resolved Hide resolved
src/CSET/operators/filters.py Outdated Show resolved Hide resolved
src/CSET/operators/filters.py Outdated Show resolved Hide resolved
src/CSET/operators/filters.py Show resolved Hide resolved
src/CSET/operators/filters.py Outdated Show resolved Hide resolved
src/CSET/operators/filters.py Outdated Show resolved Hide resolved
src/CSET/operators/filters.py Outdated Show resolved Hide resolved
src/CSET/operators/filters.py Outdated Show resolved Hide resolved
src/CSET/operators/filters.py Outdated Show resolved Hide resolved
@daflack
Copy link
Contributor Author

daflack commented Dec 12, 2024

Converting to draft to remove all CubeList functionality as there is starting to be operator purpose creep.

@daflack daflack marked this pull request as draft December 12, 2024 12:55
@daflack daflack marked this pull request as ready for review December 12, 2024 15:24
@daflack
Copy link
Contributor Author

daflack commented Dec 12, 2024

Ready for review again following changes to functionality to only be applicable for cubes due to risk of operator creep. Review comments also taken into account @jfrost-mo and @jwarner8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add mask operator
3 participants