Skip to content

Behaviour change in 1.13 for expand_dims #9100

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.

Sign up for GitHub

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

Jump to bottom
Closed
eric-wieser opened this issue May 11, 2017 · 5 comments
Closed

Behaviour change in 1.13 for expand_dims #9100

eric-wieser opened this issue May 11, 2017 · 5 comments
Labels
07 - Deprecation component: numpy.lib
Milestone
1.13.0 release

Comments

@eric-wieser
Copy link
Member

eric-wieser commented May 11, 2017 •
edited
Loading

From the mailing list, this used to succeed, but now fails, thanks to these lines in #8584.

x = np.zeros((2, 3, 4))
np.expand_dims(x, 20)

This failing absolutely seems like correct behaviour to me - unfortunately, correctness is not the metric we get to use, as this breaks matplotlib (matplotlib/matplotlib#8610).

Evidently, we need to restore the old behaviour, but with a deprecation warning
@eric-wieser eric-wieser added this to the 1.13.0 release milestone May 11, 2017
@eric-wieser eric-wieser mentioned this issue May 11, 2017
Merged
@njsmith
Copy link
Member

njsmith commented May 11, 2017

For anyone else confused by this: it sounds like the old behavior was not to insert multiple dimensions so that the resulting array indeed had 20 dimensions, but rather to silently pretend the call said np.expand_dims(a, a.ndim). Definitely a bit surprising and error prone.

Also definitely something that needs a nice long deprecation cycle to fix though...

@charris
Copy link
Member

charris commented May 17, 2017

@eric-wieser Has this been fixed?

@eric-wieser
Copy link
Member Author

No. Would appreciate if someone else could take this on. Should be a simple fix to limit the input to a.ndim

@charris
Copy link
Member

charris commented May 17, 2017

I note that normalize_axis_index is not imported into the np. root. Was that intentional?

@eric-wieser
Copy link
Member Author

eric-wieser commented May 17, 2017 •
edited
Loading

Mainly just that adding a public function would probably incur a mailing list discussion - whereas adding an internal helper does not. In particular, normalize_axis_index does not support axis tuples, which might be expected were it public

@charris charris mentioned this issue May 17, 2017
Merged
charris added a commit to charris/numpy that referenced this issue May 18, 2017
@charris
DEP: Deprecate incorrect behavior of expand_dims.
beac50c 
Expand_dims works as documented when the index of the inserted NewAxis
in the resulting array satisfies -a.ndim - 1 <= index <= a.ndim.
However, when index > a.ndim index is replaced by a.ndim and, when
index < -a.ndim - 1, it is replaced by index + a.ndim + 1, which may be
negative and results in incorrect placement. The latter two cases are
now deprecated.

Closes numpy#9100.
charris added a commit to charris/numpy that referenced this issue May 18, 2017
@charris
DEP: Deprecate incorrect behavior of expand_dims.
7415596 
Expand_dims works as documented when the index of the inserted NewAxis
in the resulting array satisfies -a.ndim - 1 <= index <= a.ndim.
However, when index > a.ndim index is replaced by a.ndim and, when
index < -a.ndim - 1, it is replaced by index + a.ndim + 1, which may be
negative and results in incorrect placement. The latter two cases are
now deprecated.

Closes numpy#9100.
@charris charris mentioned this issue May 18, 2017
Merged
mherkazandjian pushed a commit to mherkazandjian/numpy that referenced this issue May 30, 2017
@charris @mherkazandjian
DEP: Deprecate incorrect behavior of expand_dims.
838de60 
Expand_dims works as documented when the index of the inserted NewAxis
in the resulting array satisfies -a.ndim - 1 <= index <= a.ndim.
However, when index > a.ndim index is replaced by a.ndim and, when
index < -a.ndim - 1, it is replaced by index + a.ndim + 1, which may be
negative and results in incorrect placement. The latter two cases are
now deprecated.

Closes numpy#9100.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
07 - Deprecation component: numpy.lib
Projects
None yet
Milestone
1.13.0 release
Development

No branches or pull requests
3 participants
@charris @eric-wieser @njsmith