Skip to content

MAINT: Don't internally use the one-argument where #9214

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 1 commit into from
Jun 3, 2017

Conversation

eric-wieser
Copy link
Member

nonzero is a clearer spelling

@juliantaylor
Copy link
Contributor

I don't agree that nonzero is clearer on boolean inputs. where is also used in e.g. IDL.
where_nonzero would be better but I don't think another alias is a good idea either.

@eric-wieser
Copy link
Member Author

eric-wieser commented Jun 3, 2017

My argument would be that where is ambiguous, as you have to count the number of arguments to know what it does.

On the whole, it seems pretty unpythonic to have two basically unrelated functions share the same name - the only thing in common between where(c, x, y) and nonzero(c) is that the cast their first argument to bool. The only exception to this rule in the standard library that I can think of is type(var) vs type(name, bases, dict)

Obviously it's far too late to change our public API there, but I think we should at least discourage the use of np.where(c) and avoid it internally

@charris
Copy link
Member

charris commented Jun 3, 2017

Yeah, nonzero isn't the best name and where_nonzero would probably be better, but it is entrenched. The dual use of where always seemed weird to me.

@charris charris merged commit 48b0374 into numpy:master Jun 3, 2017
@charris
Copy link
Member

charris commented Jun 3, 2017

Thanks Eric.

@eric-wieser
Copy link
Member Author

Alternatively, perhaps where(c, x, y) has the wrong name. If we want to implement #8994 without dropping support for where(c), then we need a new name for that anyway, so it can be used by __array_ufunc__ (although I guess np.where.ufunc could be that name)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants