Skip to content

Conversation

@rieder
Copy link
Member

@rieder rieder commented Oct 1, 2025

Ensures that x and y use the same unit, which is otherwise ignored (it is divided out).

Ensures that x and y use the same unit, which is otherwise ignored (it is divided out).
@rieder rieder requested a review from a team as a code owner October 1, 2025 09:24
@rieder rieder requested a review from LourensVeen October 1, 2025 09:32
LourensVeen
LourensVeen previously approved these changes Oct 1, 2025
Copy link
Member

@LourensVeen LourensVeen left a comment

Choose a reason for hiding this comment

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

Whoops! Yes, that seems like a good idea. Maybe add a regression test for it?

@rieder
Copy link
Member Author

rieder commented Oct 1, 2025

Added tests and fixed a regression :)

@rieder rieder requested a review from LourensVeen October 1, 2025 11:51
arctan2 = lambda x, y: numpy.arctan2(x, y) | rad
arctan2 = lambda x, y: numpy.arctan2(
x.value_in(x.unit) if isinstance(x, quantities.Quantity) else x,
y.value_in(x.unit) if isinstance(x, quantities.Quantity) else y,
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this explicitly checks the unit of x in both cases, and also whether x is a Quantity. This is because x and y need to be treated in the same way, otherwise they might have different units.

Copy link
Member

@LourensVeen LourensVeen left a comment

Choose a reason for hiding this comment

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

Ah, hadn't considered that. I should spend more time using AMUSE I guess 😄. Nice find though! Hurray for testing things.

@rieder rieder merged commit ce21df1 into main Oct 2, 2025
2 of 3 checks passed
@rieder rieder deleted the rieder-patch-trigo branch October 2, 2025 08:57
@rieder rieder changed the title Make arctan2 work with ScalarQuantities Make arctan2 work with Quantities Oct 2, 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.

3 participants