iframe-proxy

madphysicist · 2022-01-04T06:57:24Z

This is a first attempt to fix some of #20705. In this PR, I attempt to make the following two changes to np.ndarray.view:

If dtype is smaller than the current dtype, and it divides the current one evenly, and there is at least one unit dimension, expand the last unit dimension instead of crashing.
If dtype is larger than the current dtype, and there exists an axis along which the current array is contiguous (stride == current dtype.itemsize), use that axis instead of crashing.

Both cases are valid because the check for dtype multiplicative compatibility is still done, and the dimension/stride checks guarantee that the resulting shape changes do not exceed known owned memory.

One possible modification is that we may want to restrict this only to the last dimension. Will let reviewers with more understanding decide.

This PR will allow a non-hacky rewrite of most of the functionality of #20694. If accepted, the second half of the functionality will be allowing as_strided to modify the offset and dtype of an array, in addition to its strides and shape.

Edit by seberg, posting the summary of the changes for Fortran order arrays here, in case someone comes here from the release notes:

Behavior deprecated in NumPy 1.11.0 allowed the following counterintuitive result::

 >>> x = np.array(["aA", "bB", "cC", "dD", "eE", "fF"]).reshape(1, 2, 3).transpose()
 >>> x.view('U1')  # deprecated behavior, shape (6, 2, 1)
 DeprecationWarning: ...
 array([[['a'],
         ['d']],
 
        [['A'],
         ['D']],
 
        [['b'],
         ['e']],
 
        [['B'],
         ['E']],
 
        [['c'],
         ['f']],
 
        [['C'],
         ['F']]], dtype='<U1')

Now that the deprecation has expired, dtype reassignment only happens along the
last axis, so the above will result in::

 >>> x.view('U1')  # new behavior, shape (3, 2, 2)
 array([[['a', 'A'],
         ['d', 'D']],
 
        [['b', 'B'],
         ['e', 'E']],
 
        [['c', 'C'],
         ['f', 'F']]], dtype='<U1')

When the last axis is not contiguous, an error is now raised in place of the DeprecationWarning::

 >>> x = np.array(["aA", "bB", "cC", "dD", "eE", "fF"]).reshape(2, 3).transpose()
 >>> x.view('U1')
 ValueError: To change to a dtype of a different size, the last axis must be contiguous

The new behavior is equivalent to the more intuitive::

 >>> x.copy().view('U1')

To replicate the old behavior on F-but-not-C-contiguous arrays, use::

 >>> x.T.view('U1').T

mhvk

@madphysicist - yes to finally being able to split on the last axis if that axis is contiguous rather than require the whole array to be contiguous!

But I am torn about the idea of allowing one to split on any contiguous axis with size 1; if we allow that, then I do not see why the dimension should be unity - all that should be required is that it is the contiguous axis. But it seems that led to real confusion about that for arrays that are contiguous in more than one axis (and the choice between F and C contiguous).

Looking at your test cases, I found the ones that did not work on the last axis confusing - it really needed thinking why they worked as they did.

More generally, I think I'd prefer for arr.view(...) to always do exactly the same as arr.copy().view(...), which is not true when splitting on any axis but the last.

Given this, my preference would thus be to stick to just allowing the final axis. Does that solve the character slicing issues?

p.s. I should add that mostly I want this to happen! In the end, any time one does a .view(dtype) (which I do a lot), one better really think about it.

mhvk · 2022-01-04T23:05:05Z

eric-wieser · 2022-01-04T23:14:52Z

I agree with @mhvk, requiring contiguity of the last axis is the least surprising choice of semantics.

madphysicist · 2022-01-04T23:48:27Z

@eric-wieser @mhvk After reading your comments and looking at the mess some of the tests were, I have to agree that the sane approach is to only allow the last axis to change. Transposes are cheap nowadays, so this should not be an issue. It certainly does not hold back anything in #20694: strings are pretty much guaranteed contiguous along the last axis.

Please see the updated code. It's much simpler, and the counter-intuitive tests are now gone, replaced by a single test showing that f-contiguous layouts still raise an error (with an updated message though).

madphysicist · 2022-01-05T00:11:54Z

I don't think I can fix the linter error: it's a doctest with an exception that's just too long to fit in 72 chars.

eric-wieser · 2022-01-05T00:12:21Z

This looks great now. Can you add a release note? You should mention that as a result of this change, the formerly-deprecated action of .view on fortran-order arrays now does something different.

madphysicist · 2022-01-05T00:21:29Z

@eric-wieser Two questions: 1. what do I do with the broken smoketest? 2. what version is this going into? 1.21.1?

eric-wieser · 2022-01-05T00:30:40Z

what do I do with the broken smoketest?

Remove it, it's testing the deprecation that you intentionally (and justifiably) removed:

=================================== FAILURES ===================================
__________ TestNonCContiguousViewDeprecation.test_fortran_contiguous ___________

self = <numpy.core.tests.test_deprecations.TestNonCContiguousViewDeprecation object at 0x7fa4cf0247c0>

    def test_fortran_contiguous(self):
>       self.assert_deprecated(np.ones((2,2)).T.view, args=(complex,))
E       ValueError: To change to a dtype of a different size, the last axis must be contiguous

self       = <numpy.core.tests.test_deprecations.TestNonCContiguousViewDeprecation object at 0x7fa4cf0247c0>

../../builds/venv/lib/python3.8/site-packages/numpy-1.23.0.dev0+355.g163c17aed-py3.8-linux-x86_64.egg/numpy/core/tests/test_deprecations.py:270: ValueError

(from the logs)

mhvk

Looks great now! Thanks for tackling this long-standing nuisance - I think this indeed ended up with a nice & clean solution!

madphysicist · 2022-01-05T03:37:14Z

I don't think I can reasonably fix the linter error, and the azure pipeline error appears to be unrelated to this PR. I think this PR is ready to go. @mhvk, @eric-wieser, @mattip thank you very much for your help and support in this. Glad we came up with a solution we can all agree on.

mattip · 2022-01-05T04:38:37Z

Just ignore the single lint error, it is too hard to fix.

madphysicist · 2022-01-05T06:47:39Z

The Azure error is strange. It's a different error from last time, but the only difference is the updated version string. I'm guessing it's not related to this PR. Why do seemingly random errors show up periodically?

eric-wieser

I'll stop bike-shedding the docs now, and let someone more involved in recent handling of deprecations take over. This looks great to me, and I'm surprised no one took this on earlier!

mattip · 2022-01-05T16:54:45Z

LGTM. We can always tweak the documentation going forward. For other reviewers: ignore the nit-picky linting error.

madphysicist · 2022-01-05T16:56:32Z

@eric-wieser. I suspect no one expected it to be this simple. I expected to have to modify both view and as_strided as a minimum, and not just shorten the code on a single set descriptor.

I still want to add offset and dtype arguments to as_strided. It's still affected by this contiguity check because it uses asarray under the hood, but I really don't think it should be. Do you have any thoughts on that?

eric-wieser · 2022-01-05T17:40:00Z

I still want to add offset and dtype arguments to as_strided. It's still affected by this contiguity check because it uses asarray under the hood, but I really don't think it should be. Do you have any thoughts on that?

Let's discuss this in the original PR that motivated this one, #20694.

mhvk

Looks all great to me. Probably good to squash the commits, though.

madphysicist · 2022-01-05T18:47:38Z

@mhvk Done. I probably should have split off the deprecation expiry into a separate commit, but it's too late now.

Expires deprecated F-contiguous behavior. Simplifies C code of dtype set descriptor. Adds tests that verify which error condition is triggered. Introduces extra long exception message that upsets linter.

madphysicist · 2022-01-05T20:05:58Z

Looks like everything besides the linter is passing.

mattip · 2022-01-06T07:20:05Z

Thanks @madphysicist. Github has a "squash and merge" button, so in the future you don't need to necessarily squash commits down to one, we can do it when we merge.

madphysicist · 2022-01-06T15:52:35Z

@mattip. I'll keep that in mind for the future. Which release is this going into by the way? The obspy issue that references this seems to think that it's 1.22.1, while you asked me to set the .. versionchanged:: to 1.23.0

mattip · 2022-01-06T16:15:22Z

It is up to the release manager. If @charris backports this to 1.22 then we should change the version changed appropriately.

seberg · 2022-01-06T16:17:36Z

Since this is a (nice!) enhancment and finalized a deprecation, it would normally not be backported. Which doesn't mean that there could not be an exception if it is particularly important.

charris · 2022-01-06T16:21:22Z

See obspy/obspy#2921 — apparantly this is solved in numpy/numpy#20722

github-actions Bot added the 01 - Enhancement label Jan 4, 2022

madphysicist commented Jan 4, 2022

View reviewed changes

Comment thread numpy/core/src/multiarray/getset.c

madphysicist commented Jan 4, 2022

View reviewed changes

Comment thread numpy/lib/stride_tricks.py

madphysicist commented Jan 4, 2022

View reviewed changes

Comment thread numpy/core/src/multiarray/getset.c Outdated

madphysicist marked this pull request as ready for review January 4, 2022 08:00

madphysicist mentioned this pull request Jan 4, 2022

ENH: Added np.char.slice_ #20694

Closed

mattip reviewed Jan 4, 2022

View reviewed changes

Comment thread numpy/core/_add_newdocs.py

mattip reviewed Jan 4, 2022

View reviewed changes

Comment thread numpy/core/tests/test_multiarray.py Outdated

mhvk reviewed Jan 4, 2022

View reviewed changes