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

BUG: Power override for zero power #204

Merged
merged 6 commits into from
Oct 12, 2021

Conversation

JBorrow
Copy link
Contributor

@JBorrow JBorrow commented Oct 6, 2021

Implements a fix to #203.

Comments welcome as this could be seen as quite a dirty fix, as we're taking away ufunc dispatch from numpy.

@ngoldbaum
Copy link
Member

With this PR if you take a Unit object to a zeroth power, do you get back dimensionless? I suspect you don’t since the content of __pow__ is unchanged. Since numpy had an optimization that shortcuts to ones_like you may need to make changes both to the unyt_array class and to the Unit class to make their behavior consistent. I would also like to see a test for the behavior of the Unit class independent of the behavior of the unyt_array class. I would also move the comment change to the unit object’s __pow__ method to the new method you added to unyt_array. The unit object doesn’t know anything about numpy’s ufunc system.

Copy link
Contributor Author

@JBorrow JBorrow left a comment

Choose a reason for hiding this comment

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

At the moment the Unit object happily goes to the zeroth power (I've added a test for this, though). The unit calculation internally is completely fine (and always has been); sympy handles the 0th power gracefully.

The base issue here is that we have the _ones_like ufunc mapped to _preserve_units (which makes sense,

_ones_like: _preserve_units,
). Unfortunately this same ufunc is used for behaviours that aren't consistent when units are associated with the data.

I've moved around the comments as requested.

@JBorrow JBorrow changed the title Power override for zero power BUG: Power override for zero power Oct 12, 2021
@ngoldbaum
Copy link
Member

Thanks for clearing up where the issue was and updating the tests and comments!

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