-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
ENH: add identity kwarg to frompyfunc #8255
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
Conversation
I created a short script to demonstrate what happens with different identity values. It needs the identity kwarg added in this PR. It shows the get/set value needs work. |
Haven't looked at this yet, but the enhancement would be useful. |
The only two that are omited are |
That might make sense. Should I also add a private setter function to convert from input to the internal constants? I think ultimately for some input identity value inp, np.frompyfunc(f, nin, nout, inp).identity == inp (or raise an exception) for all possible values of inp. |
At the very least, it'd be nice to get the change supporting kwargs to |
☔ The latest upstream changes (presumably #8861) made this pull request unmergeable. Please resolve the merge conflicts. |
Seem to me that we first want to overhaul I've a patch |
#8955 has been merged, @mattharrigan would you like to revive this? |
Rebased and solved conflicts. Tweaked the |
types, /* ntypes */ 1, nin, nout, PyUFunc_None, | ||
str, doc, /* unused */ 0); | ||
types, /* ntypes */ 1, nin, nout, identity ? PyUFunc_IdentityValue : PyUFunc_None, | ||
str, doc, /* unused */ 0, NULL, identity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A future PR could add signature
, which would help vectorize
Test added |
Thank Eric and @mattharrigan. The signature addition has been on my/our "Not Done" list for a bit. Should fairly straight forward, but requires a new slightly different inner loop to handle. |
@eric-wieser feel free to merge, I think this is good. There are tiny changes in that arguments are now not only positional (which is good). I guess a ping to the mailing list can be good, but since this is a core ufunc feature exposing it to python seems very straight forward. I feel the argument description is a bit difficult, but I cannot figure out how to make it easier. Maybe the link to the ufunc documentation is simply enough. |
Hmm, seems I could have just merged it back then. I think it is an obvious enough addition to have the |
PR to Close #8245. Adding the kwarg seems pretty straightforward. Understanding what identity actually does is much harder. Several identity constants are defined here. The fact that MinusOne is defined to 2 and None is defined to -1 makes my head hurt. The ufunc getter for identity here seems to have fewer options than what's defined in the earlier link. Finally when I look at the reduce methods they don't make sense to me either. Is there any documentation defining all the possible values for identity and what they mean?
~WIP: ~ (edit by @eric-wieser: rebased and finished)