8000 ENH: Allow moveaxis() to take strings as axis order by manuels · Pull Request #11504 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

ENH: Allow moveaxis() to take strings as axis order #11504

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
wants to merge 1 commit into from
Closed

ENH: Allow moveaxis() to take strings as axis order #11504

wants to merge 1 commit into from

Conversation

manuels
Copy link
@manuels manuels commented Jul 5, 2018 •
edited
Loading

Adds a feature to simplify moving axes around:

x = np.zeros((0, 1, 2))
np.moveaxis(x, 'xyz', 'zyx')
np.moveaxis(x, 'ijk', 'kji') # has the same effect as above

@mattip
Copy link
Member
mattip commented Jul 5, 2018

Tests and handling the case where the strings do not match are missing.

This seems like a nice addition, should it hit the mailing list since it is a new feature?

@eric-wieser
Copy link
Member

Note you can already spell this np.einsum('ijk->kji', x).

Definitely needs to hit the mailing list. I'm +0 on this - it seems reasonable, but does violate "There should be one-- and preferably only one --obvious way to do it"

@manuels
Copy link
Author
manuels commented Jul 5, 2018

@mattip, I just added the handling and test. I will write to the mailing list as soon as I get the subscription notification.

There should be one (...) obvious way to do it"

@eric-wieser You are right, but I think permuting axes using einsum() is not the most obvious way.

@eric-wieser
Copy link
Member

I was referring to np.moveaxis([0, 1, 2], [2, 1, 0]) as the obvious way. I agree that einsum is not an obvious way to spell that.

@eric-wieser
Copy link
Member
eric-wieser commented Jul 5, 2018 •
edited
Loading

Faster too:

a = np.zeros((10, 20, 30))

%timeit np.einsum('abc->cba', a)
917 ns ± 105 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

%timeit np.moveaxis(a, [0, 1, 2], [2, 1, 0])
7.92 µs ± 705 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

@mattip
Copy link
Member
mattip commented Jul 5, 2018

Parsing in python vs. parsing in C strikes again (for functions that do very little work). Benchmarking np.transpose(a, [2, 1, 0]) (which np.moveaxis eventually calls after parsing its arguments) is ~10% faster than einsum

Perhaps this PR could be reframed as mentioning einsum and character parsing in the docstring.

@manuels
Copy link
Author
manuels commented Jul 5, 2018 •
edited by eric-wieser
Loading

Perhaps this PR could be reframed as mentioning einsum and character parsing in the docstring.

@mattip, I could add einsum to the docstring, but I have the feeling that Einstein's sum convention is very little known outside of the field of physics which makes code using einsum much harder to read for a lot of people than moveaxis. And even in physics it's mainly particle physics IMHO.

@eric-wieser, oh, ok. I thought you meant einsum. My bad!

@eric-wieser
Copy link
Member

And even in physics it's mainly particle physics IMHO.

@manuels: My introduction to einstein summation was for continuum mechanics for structural engineering, so I think that's a unfair generalization.

@mhvk
Copy link
Contributor
mhvk commented Jul 5, 2018

Thanks for the contribution! At first I thought it was quite cute, but thinking more about it I'd agree with @eric-wieser that it is better to just have one way to use moveaxis. The problem with allowing multiple ways is that while it may be nice if a given project uses, say, the string method consistently, I think this doesn't outweigh the cognitive dissonance for people who work on multiple projects who encounter multiple ways to do the same thing.

p.s. I also don't think referring to einsum is all that useful, since that makes a copy instead of taking a new view.

p.s.2 Speed is improved by ~30% by avoiding the try/except with a simple if not instance(axis, (list, tuple)) in normalize_axis_tuple (where currently iterables take the costly except path). Not sure if this is worth a PR...

@mattip
Copy link
Member
mattip commented Jul 5, 2018

I also don't think referring to einsum is all that useful, since that makes a copy instead of taking a new view

Are you sure?

a = np.ones((10, 20, 30))
b = np.einsum('ijk->kij', a)
b.base is a  # True

@mhvk
Copy link
Contributor
mhvk commented Jul 5, 2018

OK, in that case I do think it is useful to refer to einsum!

@eric-wieser
Copy link
Member

Not sure if this is worth a PR...

I think it probably is - switching all internal uses of rollaxis to moveaxis came with a cost, and it would be nice to partially make up for that

@manuels
Copy link
Author
manuels commented Jul 6, 2018

@eric-jones, sorry, looks like I got the wrong impression about the spread of Einstein's convention.

p.s.2 Speed is improved by ~30% by avoiding the try/except with a simple if not instance(axis, (list, tuple)) in normalize_axis_tuple (where currently iterables take the costly except path). Not sure if this is worth a PR...

@mhvk, how do you mean? The only try/except statement I use is to catch a real error.

Is there a problem with the mailinglist server? I registered twice but I never got a subscription notification email.

@eric-wieser
Copy link
Member

@manuels: @mhvk is commenting on the internals of np.core.normalize_axis_tuple, not this patch. Wrong eric again, by the way.

@mhvk
Copy link
Contributor
mhvk commented Jul 6, 2018

See #11518 for the speed-up PR. Not 100% happy with it, but it is significantly faster.

@manuels manuels mentioned this pull request Jul 7, 2018
Closed
@seberg
Copy link
Member
seberg commented Jul 8, 2018 •
edited
Loading

Should just open an issue instead maybe. But is it just me or anyone else finds it very strange and potentially (though a bit concieved) dangerous that einsum does not creating a copy in all cases?

EDIT: Even overzealous:

In [6]: a = np.arange(10).reshape(5, 2)

In [7]: np.einsum('ij->ij', a, order='F').flags
Out[7]: 
  C_CONTIGUOUS : True
  F_CONTIGUOUS : False

Anyway, I think this is a premature and surprising optimization and should be removed, unless we think it might break someones code (my opinion and I admit this is actually documented further down in the function).

@mattip mattip added component: numpy._core 57 - Close? Issues which may be closable unless discussion continued component: numpy.einsum labels Jul 25, 2018
@manuels
Copy link
Author
manuels commented Aug 1, 2018

So, there hasn't been any response on the mailing list. Either that's out of interest or numpy devs are ambivalent with respect to this feature.

@eric-wieser
Copy link
Member

@seberg: Perhaps a copy parameter would be sensible. If you think that's worth further discussion, can I suggest you open a more discoverable issue about it?

@eric-wieser eric-wieser mentioned this pull request Aug 1, 2018
Open
@eric-wieser
Copy link
Member

@manuel: If the mailing list is ambivalent, and we're leaning very slightly against adding it, I think we should stick to the status quo. Thanks for trying to make numpy better though - you kicked off some useful discussions!

@eric-wieser eric-wieser closed this Aug 1, 2018
@mhvk mhvk mentioned this pull request Feb 2, 2019
Open
640C
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
57 - Close? Issues which may be closable unless discussion continued component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@manuels @mattip @eric-wieser @mhvk @seberg
0