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: fix inheritance in unyt_array.__pow__ #249

Merged
merged 2 commits into from
Jul 18, 2022

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Jul 15, 2022

This change is a solution to a downstream issue in yt:
yt-project/yt#4023

it's a direct follow up to #204

I'm not 100% sure it's the right solution, as I don't understand the original implementation using __array_func__, so maybe I'm just missing something, in which case the problem should be resolved on the yt side instead.

@JBorrow I know it's been a little while, any chance you could explain your initial implementation please ?

@neutrinoceros neutrinoceros added the bug Something isn't working label Jul 15, 2022
@neutrinoceros
Copy link
Member Author

neutrinoceros commented Jul 18, 2022

So I digged a little further and I think I get why it was implemented this way: given arr a unyt_array instance, as of unyt 2.8, the __pow__ protocol was only called for arer**x if x==0, and it's the only case that was problematic (#203). However the situation changes if unyt_array overrides the base __pow__ protocol from np.ndarray, and now __pow__ gets called for every value of x. @JBorrow very likely understood this and chose an implementation that made sense to preserve backward compatibility, redirecting to self.__array_ufunc__ for x != 0.

Subclassing np.ndarray proves however more complicated than expected, and this did not actually preserve backward compatibility as seen in yt yt-project/yt#4023
My corrected implem leaves all responsabilities to numpy and works better in that sense.

While I was researching this problem, I also realised that the __pow__ method normally takes an optional mod argument https://numpy.org/doc/stable/reference/generated/numpy.ndarray.__pow__.html?highlight=__pow__#numpy.ndarray.__pow__
so I "fixed" this too (using quotation marks here because I couldn't find a single working example using this argument, even with pure np.ndarrays)

@JBorrow
Copy link
Contributor

JBorrow commented Jul 18, 2022

Yes, that's effectively it. Basically, it's not clear what ufunc numpy is actually using when you do a**n, and I chose to use the unyt subclassing incase we overwrote anything. If we don't, then this is fine.

@neutrinoceros
Copy link
Member Author

Thank you very much !
@jzuhone I think this is now ready :)

@jzuhone jzuhone merged commit db61a63 into yt-project:master Jul 18, 2022
@neutrinoceros neutrinoceros deleted the hotfix_pow branch July 18, 2022 14:35
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.

3 participants