-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
MAINT: flake8 cleanups #11909
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: flake8 cleanups #11909
Conversation
numpy/core/arrayprint.py
Outdated
@@ -1346,7 +1346,7 @@ def dtype_is_implied(dtype): | |||
return dtype.type in _typelessdata | |||
|
|||
|
|||
def dtype_short_repr(dtype): | |||
def dtype_short_repr(_dtype): |
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.
Trailing underscore would probably be better here and consistent with other usage in this module.
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.
fixed by removing the import
Minor style nit. |
Why not just remove the import of |
b7a4018
to
5e3412d
Compare
@@ -18,7 +18,7 @@ | |||
_fastCopyAndTranspose as fastCopyAndTranspose, ALLOW_THREADS, | |||
BUFSIZE, CLIP, MAXDIMS, MAY_SHARE_BOUNDS, MAY_SHARE_EXACT, RAISE, | |||
WRAP, arange, array, broadcast, can_cast, compare_chararrays, | |||
concatenate, copyto, count_nonzero, dot, dtype, empty, |
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.
I'm not sure we can remove this.
I started trying to go down the road of trying to clear up imports, and realized that we have a problem - we'll break anyone using np.core.numeric.count_nonzero
in their code. It seems to me we're stuck importing piles of junk...
The alternative would be to declare "if it's not in __all__
, it's subject to move at any time."
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.
+1 for removing things not in __all__
. It is so much simpler to spell np.core.numeric.count_nonzero
as np.count_nonzero
. Does this need a release note? We should at least have a clear namespace policy - do we continue to hold onto np.core.numeric
, np.core.multiarray
, np.core.umath
or do we export names directly from np
?
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.
Perhaps this should hit the mailing list.
Perhaps the easiest option is just to:
- move
np.core.numeric
tonp.core._numeric
, and similar for other modules within it. The underscore would make the "don't rely on this" intent - create a new
np.core.numeric
that:- emits a warning upon import
- preserves the same api as we currently have
This would give us the freedom to come up with a more logical file structure, without risk of breaking anything.
I'd still advocate leaving np.core
around as a place for semi-public internals for other infrastructure packages (ie np.core.normalize_axis_index
)
I think such a proposal would probably be NEP-worthy
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.
IIRC, ma.testutils
suffers from that problem, and lots of people were importing function from there that they shouldn't have.
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.
+1 for Eric's proposal
I think such a proposal would probably be NEP-worthy
agree
The alternative would be to declare "if it's not in
__all__
, it's subject to move at any time."
In principle I'd say:
- the bar is a lot lower there
- we shouldn't (re)move things gratuitously
- it's okay to do so though if it would take a lot of work to keep non-public API in place or if it would limit evolution of the code base.
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.
NEP draft at #11924.
I'd still like to see the name |
Thanks Matti. |
running
produced a number of places where we duplicate imports and variable names