Skip to content

DEP: deprecate passing a generator to stack functions #12280

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
Oct 29, 2018

Conversation

shoyer
Copy link
Member

@shoyer shoyer commented Oct 27, 2018

Fixes #12263

We can't support generators with dispatch for __array_function__.

@mattip
Copy link
Member

mattip commented Oct 28, 2018

Does this fix the pandas/scipy issues with NumPy HEAD?

@ilayn
Copy link
Contributor

ilayn commented Oct 28, 2018

I think the warning is a bit too technical and would be cryptic to many users (including me if I didn't know what the issues was). A gist of the issue would be sufficient in my opinion. Users who are utilizing __array_function__ would probably find their way out themselves. Something along the following would be better since it mentions objects that are familiar to everyone.

              'arrays to stack must be passed as a "sequence" type such '
              'as lists, tuples and so on. Support for non-sequence '
              'iterables such as generators, is deprecated as of NumPy '
              '1.16 and will raise an error in the future.',
              FutureWarning, stacklevel=stacklevel)

@shoyer shoyer force-pushed the stack-generator-warning branch from b2bd14a to 0064021 Compare October 28, 2018 21:59
@shoyer
Copy link
Member Author

shoyer commented Oct 28, 2018

@ilayn good point, I adjusted the warning text to your suggestion. I agree that it's probably not necessary to mention __array_function__ here.

@mattip It definitely fixes pandas on NumPy head. I don't have local dev installs of scikit-learn or scipy setup, so I can't confirm for those yet.

@tylerjereddy
Copy link
Contributor

SciPy has already merged the compensating changes that remove generator usage in this context so that our CI remains "green."

I restarted the buggy shippable builds here

@charris
Copy link
Member

charris commented Oct 29, 2018

Why FutureWarning when the message says "deprecated"? Just Deprecate the sucker.

'such as list or tuple. Support for non-sequence '
'iterables such as generators is deprecated as of '
'NumPy 1.16 and will raise an error in the future.',
FutureWarning, stacklevel=stacklevel)
Copy link
Member

@eric-wieser eric-wieser Oct 29, 2018

Choose a reason for hiding this comment

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

This raises an interesting point - is wrapping everything in array_function_dispatch going to make all our warnings be emitted within the __array_function__ dispatcher?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, yes.

In general: warnings about changed functions arguments/signatures should go in the dispatcher, since they are relevant whether you are using NumPy or not. Warnings about changed function behavior should remain in the implementations.

@shoyer
Copy link
Member Author

shoyer commented Oct 29, 2018

Why FutureWarning when the message says "deprecated"? Just Deprecate the sucker.

FutureWarning is now described as "Base category for warnings about deprecated features when those warnings are intended for end users of applications that are written in Python."

But I guess I'm still used to the old policy: "Changed in version 3.7: Previously DeprecationWarning and FutureWarning were distinguished based on whether a feature was being removed entirely or changing its behaviour. They are now distinguished based on their intended audience and the way they’re handled by the default warnings filters."

I was thinking of this as a behavior change, but I suppose that isn't exactly true -- we will be raising an error instead.

Do we have a NumPy specific policy about when we use DeprecationWarning vs VisibleDeprecationWarning vs FutureWarning?

@charris
Copy link
Member

charris commented Oct 29, 2018

Do we have a NumPy specific policy about when we use DeprecationWarning vs VisibleDeprecationWarning vs FutureWarning?

Sounds like VisibleDeprecationWarning should derive from FutureWarning at some point in the future. I think this is a consequence of Python hiding DeprecationWarning by default, a change that was a kludge to fix the mistake of issuing a DeprecationWarning in Python 2.7 about some change in Python 3. Or at least, that is how I remember it :) To be clear, VisibleDeprecationWarning is used when we want everyday users to see the warning by default, DeprecationWarning is for downstream developers who may only see it in development testing, and FutureWarning for changed behavior. I suppose we can adopt the Python 3.7 convention at some point, but we will need to have a way to signal changed behavior vs disappearing features.

@charris
Copy link
Member

charris commented Oct 29, 2018

Sounds like we need to document the use of DeprecationWarning someplace.

@charris charris merged commit e726c04 into numpy:master Oct 29, 2018
@charris
Copy link
Member

charris commented Oct 29, 2018

Thanks Stephan.

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.

6 participants