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

MAINT: Made iterable return a boolean #7202

Merged
merged 1 commit into from
Feb 11, 2016

Conversation

madphysicist
Copy link
Contributor

iterable in numpy/lib/function_base.py currently returns an integer 0 or 1. There is no reason not to return the more Pythonic True or False. All usages within numpy are in statements of the form if itrerable(...): and if not iterable(...):. Backwards compatibility outside numpy will not be broken since True==1 and False==0.

@njsmith
Copy link
Member

njsmith commented Feb 7, 2016

WTF, why do we have np.iterable as a top-level exposed function.

@madphysicist
Copy link
Contributor Author

No idea. It's the first thing in numpy/lib/function_base.py and it uses 0 and 1 as its return values, so many guess is it's from some obscenely old version of numpy. It is used in relatively few places in numpy (certainly fewer than a dozen), and can probably be completely eliminated internally. Also, it does not appear to be present anywhere in the documentation. On the downside, it does appear in __all__.

It seems that the MO in this case would be deprecation? In that case, I can investigate whether the function can be eliminated from numpy internally. If so, it can be deleted completely instead of just prepending an underscore and reviving from __all__.

@madphysicist
Copy link
Contributor Author

In the meantime, I think this PR is still valid given the circumstances.

@njsmith
Copy link
Member

njsmith commented Feb 7, 2016

Hah, yeah, check it out: 052a7b2.

It would make sense to deprecate the public exposure, I guess. Not really the highest priority thing. Removing internal uses is likely to be trickier, and not sure it's worth bothering.

b : {0, 1}
Return 1 if the object has an iterator method or is a sequence,
and 0 otherwise.
b : {False, True}
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: should be b : bool

@madphysicist
Copy link
Contributor Author

I added a DeprecationWarning in addition to the other fixes.

@njsmith
Copy link
Member

njsmith commented Feb 7, 2016

You can't just deprecate a function used internally without making the tests start failing.

Also the deprecation should probably get discussed on the mailing list, even if it is a silly function to be exporting in the first place. On further thought, honestly, I personally don't really have the attention to spend a lot of time worrying about this silly little iterable function right now, so unless someone else wants to step up and help shepherd it through, or it's something that you think you'd get something valuable out of working on (maybe just the experience of going through the process?), then I'd suggest just putting off the deprecation until later.

@charris
Copy link
Member

charris commented Feb 8, 2016

Let's just fix the function. Besides, I have a severe allergy to all the try .. except constructs.

@madphysicist madphysicist force-pushed the iterable-bool-return branch 2 times, most recently from 66d0a30 to 8181f69 Compare February 8, 2016 02:52
@jakirkham
Copy link
Contributor

Failures on Travis CI appear related to annoyances from virtualenv and friends. Maybe a merge with master, rebase on master, or simply restarting the tests will resolve the issue.

@madphysicist
Copy link
Contributor Author

Looks like a rebase fixed it.

@madphysicist
Copy link
Contributor Author

I have made one more commit to cleanly deprecate the pubic function. This version does not introduce any additional try...except allergens or change any of the internal code (or break anything). If this is still too much at the wrong time, I will revert.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This is a very old function that has some limited internal use but
should not be a part of the public API. It can still be accessed
internally as `numpy.function_base._iterable`.
Copy link
Member

Choose a reason for hiding this comment

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

release notes are for public consumption, so shouldn't mention the internal API unless we want people to use it (which we don't)

@njsmith
Copy link
Member

njsmith commented Feb 11, 2016

LGTM modulo the one comment, but if deprecating we should email the list to check that no-one objects -- can you do that?
I don't expect we're likely to actually remove any time in the foreseeable future

@madphysicist
Copy link
Contributor Author

Fixed and sent.

@njsmith
Copy link
Member

njsmith commented Feb 11, 2016

thanks! can you post a link to the archives thread here so we can find it
later?

On Wed, Feb 10, 2016 at 7:10 PM, Mad Physicist notifications@github.com
wrote:

Fixed and sent.


Reply to this email directly or view it on GitHub
#7202 (comment).

Nathaniel J. Smith -- https://vorpus.org http://vorpus.org

@madphysicist
Copy link
Contributor Author

Sure, here is the original email in the archives: https://mail.scipy.org/pipermail/numpy-discussion/2016-February/074920.html

@njsmith
Copy link
Member

njsmith commented Feb 11, 2016

Thanks! Now we wait a few days and see if anyone yells... (sorry for the
hassle, and thanks for being a good sport -- this is just the standard
procedure, so you see why I was wary about taking it on for a small change
like this :-). hopefully it is at least an interesting experience for
you...)

On Wed, Feb 10, 2016 at 7:29 PM, Mad Physicist notifications@github.com
wrote:

Sure, here is the original email in the archives:
https://mail.scipy.org/pipermail/numpy-discussion/2016-February/074920.html


Reply to this email directly or view it on GitHub
#7202 (comment).

Nathaniel J. Smith -- https://vorpus.org http://vorpus.org

@madphysicist
Copy link
Contributor Author

No problem at all. This is my first real experience contributing to a major open source project. It has been great so far. I have been using numpy for a while now, and it is nice to see how it works on the inside as well as to make some contributions beyond bug reports. So far, everything has been quite sensible and I really appreciate how much time and effort is put into not breaking things for the end user.

@madphysicist
Copy link
Contributor Author

Given how often this function appears to be used, I have reverted my last commit. It is a pity this was not caught earlier (by about ten years). The code that is in the PR now should be trivial to merge.

@njsmith
Copy link
Member

njsmith commented Feb 11, 2016

Yeah, too bad. Thanks for your patience with all of this :-)

njsmith added a commit that referenced this pull request Feb 11, 2016
MAINT: Made `iterable` return a boolean
@njsmith njsmith merged commit 47b6c2b into numpy:master Feb 11, 2016
@madphysicist
Copy link
Contributor Author

Thanks. I've learned a lot with the little PRs I've been making. I appreciate the hands-on teaching you and the rest of the core developers have been doing. Now I think it's time for me to delve a little deeper into the real code and see what I can learn.

@madphysicist madphysicist deleted the iterable-bool-return branch February 12, 2016 00:58
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.

None yet

4 participants