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

Create common operators script #620

Merged
merged 12 commits into from
May 17, 2024
Merged

Create common operators script #620

merged 12 commits into from
May 17, 2024

Conversation

jwarner8
Copy link
Contributor

@jwarner8 jwarner8 commented May 16, 2024

Create common operator script to hold regularly used iris/processing functionality that isn't an operator independently

Fixes #619

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.
  • Added an entry to the top of docs/source/changelog.rst
  • Conda lock files have been updated if dependencies changed.
  • Marked the PR as ready to review.

@jwarner8 jwarner8 changed the title Create common operators script to hold regularly used iris/processing functionality that isn't an operator independently Create common operators script May 16, 2024
@jwarner8 jwarner8 added enhancement New feature or request cleanup Non-functional improvement labels May 16, 2024
@jwarner8 jwarner8 self-assigned this May 16, 2024
Copy link
Contributor

github-actions bot commented May 16, 2024

Coverage

@jwarner8
Copy link
Contributor Author

Linked #619

@jfrost-mo
Copy link
Member

If you edit the initial comment, and put the issue number after the "Fixes #" bit, it will properly link, and close the issue when this PR is merged.

@jwarner8
Copy link
Contributor Author

jwarner8 commented May 16, 2024

I put in the identification of horizontal coordinate names as a generic function useful for many operators as a start. Not convinced if _common_operators.py is the best name though, as might conflate the use of 'common' operators such as regrid which will be used in several recipes. Creative suggestions welcome!

@jfrost-mo
Copy link
Member

Not very creative, but _operator_utils.py? Or even just _utils.py, but inside the operators subdirectory.

src/CSET/_common_operators.py Outdated Show resolved Hide resolved
src/CSET/_common_operators.py Outdated Show resolved Hide resolved
tests/test_common_operators.py Outdated Show resolved Hide resolved
@jwarner8 jwarner8 marked this pull request as ready for review May 17, 2024 08:47
@jwarner8 jwarner8 requested a review from jfrost-mo May 17, 2024 08:47
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.

Code looks good. The only thing left is whether this code should be used from somewhere before it is merged. I know it will be used in your Age of Air diagnostic, but is it used anywhere else?

@jfrost-mo jfrost-mo closed this May 17, 2024
@jfrost-mo jfrost-mo reopened this May 17, 2024
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.

If its not used anywhere in the current code, we can still go ahead and merge it, and you'll want to update your Age of Air branch to take advantage of it.

@jwarner8
Copy link
Contributor Author

If its not used anywhere in the current code, we can still go ahead and merge it, and you'll want to update your Age of Air branch to take advantage of it.

After this gets merged, it will immediately be used within the regrid operator, and I will create a new PR/issue for updating this operator to use the common function. Then, I will work on the age of air review, which uses the regrid operator as part of the recipe. The cross section work will directly make use of the common function too

@jwarner8
Copy link
Contributor Author

If its not used anywhere in the current code, we can still go ahead and merge it, and you'll want to update your Age of Air branch to take advantage of it.

After this gets merged, it will immediately be used within the regrid operator, and I will create a new PR/issue for updating this operator to use the common function. Then, I will work on the age of air review, which uses the regrid operator as part of the recipe. The cross section work will directly make use of the common function too

I figured probably easier to merge this first and then update regrid, rather than update regrid operator in this PR too (so the PR's have a clear separate purpose)?

@jfrost-mo
Copy link
Member

That works. Only other thing is to add an entry to the top of the changelog (docs/source/changelog.rst), then you can go ahead and merge this.

@jwarner8 jwarner8 merged commit 7f0d911 into main May 17, 2024
7 checks passed
@jwarner8 jwarner8 deleted the common_operators branch May 17, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Non-functional improvement enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add repetitive functions to _common.py or similar.
2 participants