Skip to content

Conversation

ThatDesert
Copy link
Contributor

The function cf.Data.arctan2 now first checks if x2 has cf-style units (of the same type as the cf.Units class).

  • If x1 unitless and x2 has cf units:
    • Attempts to conform x1 to x2's units, but x1 remains unitless
    • da.arctan2() treats them both as unitless
  • If x1 has cf units and x2 unitless:
    • Does not attempt to conform x1 to x2's units
    • da.arctan2() treats them both as unitless
  • If x1 and x2 both unitless
    • Does not attempt to conform x1 to x2's units
    • da.arctan2() treats them both as unitless
  • If 'x1' and 'x2' both have cf units
    • Attempts to conform x1 to x2's units
      • If x1's units are of a different dimension than x2's, an exception is raised
    • da.arctan2() operates on x2 and the converted x1

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.

Thanks for this Ollie, really good stuff - I've requested a pair of minor tweaks but they are mostly to make the changes of the PR consistent with (e.g. terminology) in our codebase and 'house style' so nothing which could have been foreseen to do.

Please either accept those suggestions or commit something similar to address the feedback and then I can merge this. As we discussed on your placement, a test would usually be added but since we ran out of time I can get the test added after this PR is merged (or guide you through how to add it if preferred).

ThatDesert and others added 2 commits August 27, 2025 11:05
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Co-authored-by: Sadie L. Bartholomew <[email protected]>
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.

Confirmed as working from local interactive use. I will add a test to capture the previous issue in a post-merge commit. Ollie has volunteered to add the test in a follow-up PR to go up soon - thanks!

@sadielbartholomew sadielbartholomew merged commit 14c3166 into NCAS-CMS:main Aug 27, 2025
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.

2 participants