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

Field combination #785

Merged
merged 11 commits into from
Jun 18, 2024
Merged

Conversation

davidhassell
Copy link
Collaborator

Fixes #784

Notes

  • The changes to test_functions.py are left-over linting from a previous PR.

  • The changes to cf.FIeld._binary_operation look scarier than they are :). We've essentailly just moved from 2 cases (axis defined by dim coord, or axis defined by aux coord) to 3 cases (axis defined by dim coord, or axis defined by aux coord, or discrete axis), and the rest is largely just a bit code reorganisation around that (this is old and horrible code!)

  • This doesn't do what we might hope for in the DSG discrete axis case, but that's not a fault of this code. In this case, it's doing the right thing with the discrete axes, but is the non-discrete axes that unexpected broadcasting, because these axes have no identifying coordinates - that a sperate issue to be dealt with elsewhere.

@davidhassell davidhassell added the bug Something isn't working label Jun 13, 2024
@davidhassell davidhassell added this to the NEXT VERSION milestone Jun 13, 2024
Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Minor comments but overall I am happy, so please merge when you have considered those (or upon request I can do a re-review). Thanks.

Changelog.rst Outdated Show resolved Hide resolved
cf/field.py Outdated Show resolved Hide resolved
cf/field.py Show resolved Hide resolved
cf/field.py Show resolved Hide resolved
cf/field.py Show resolved Hide resolved
cf/mixin/fielddomain.py Outdated Show resolved Hide resolved
cf/mixin/fielddomain.py Show resolved Hide resolved
@sadielbartholomew
Copy link
Member

sadielbartholomew commented Jun 18, 2024

@davidhassell I've responded regarding your comments and all is clarified. Seems like we're in a state to merge now. Thanks.

@davidhassell davidhassell merged commit 7c2acd9 into NCAS-CMS:main Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combining UGRID fields erroneously creates an extra axis and broadcasts over it
2 participants