8000 ENH: add identity kwarg to frompyfunc by mattharrigan · Pull Request #8255 · numpy/numpy · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 4 commits into from
Jan 17, 2020

Conversation

mattharrigan
Copy link
Contributor
@mattharrigan mattharrigan commented Nov 8, 2016

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)

  • still need to edit release notes and frompyfunc doc
  • May need an email to numpy discussion for the questions above or simply because of the API change.
  • finally should add tests for the new functionality, but may have to wait till the getter and identity values are straightened out

@mattharrigan
Copy link
Contributor Author
mattharrigan commented Nov 9, 2016

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.

@charris
Copy link
Member
charris commented Nov 9, 2016

Haven't looked at this yet, but the enhancement would be useful.

@eric-wieser
Copy link
Member
eric-wieser commented Nov 14, 2016

The ufunc getter for identity here seems to have fewer options than what's defined in the earlier link

The only two that are omited are PyUFunc_None and PyUFunc_ReorderableNone, both of which are returned as None in ufunc.identity. Perhaps there should be two special singleton values here instead, to allow python code to distingish them?

@mattharrigan
Copy link
Contributor Author

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.

@eric-wieser
Copy link
Member
eric-wieser commented Mar 28, 2017

At the very least, it'd be nice to get the change supporting kwargs to frompyfunc in

@homu
Copy link
Contributor
homu commented Mar 28, 2017

☔ The latest upstream changes (presumably #8861) made this pull request unmergeable. Please resolve the merge conflicts.

@eric-wieser
Copy link
Member
eric-wieser commented Apr 16, 2017

Seem to me that we first want to overhaul ufunc.identity to allow any PyObject *, and only then is this worth pursuing. Under the hood, we just convert them to PyObject *s each time we need them anyway.

I've a patch in progress in my tree, but it's unlikely to get many brain cycles in the next month at #8955. Am I right in thinking that appending fields to Py_UfuncObject breaks neither the API nor the ABI?`

@mattip
Copy link
Member
mattip commented May 1, 2019

#8955 has been merged, PyUFuncObject now has a field that can hold arbitrary PyObject s

@mattharrigan would you like to revive this?

@eric-wieser
Copy link
Member

Rebased and solved conflicts. Tweaked the identity argument to be keyword-only.

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);
Copy link
Member

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

@eric-wieser eric-wieser added 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes 59 - Needs tests labels Dec 4, 2019
@eric-wieser eric-wieser removed 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes 59 - Needs tests labels Dec 6, 2019
@eric-wieser
Copy link
Member

Test added

@eric-wieser eric-wieser requested a review from seberg December 6, 2019 11:50
@seberg
Copy link
Member
seberg commented Dec 12, 2019

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.

@seberg
Copy link
Member
seberg commented Dec 12, 2019

@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.

@seberg seberg removed their request for review December 12, 2019 20:25
@seberg
Copy link
Member
seberg commented Jan 17, 2020

Hmm, seems I could have just merged it back then. I think it is an obvious enough addition to have the identity kwarg in frompyfunc since it is something we have in the C-API.

@seberg seberg merged commit bf6859c into numpy:master Jan 17, 2020
@seberg seberg added the 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified. label Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
01 - Enhancement 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified. component: numpy._core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: frompyfunc set user specified identity attribute
6 participants
0