Repository navigation
BUG: avoid mutation of shape attibutes (modeling) - #19129
Conversation
neutrinoceros
added
no-changelog-entry-needed
numpy-dev
backport-v7.2.x
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
neutrinoceros
changed the title
586db46 to
6e60f1e
Compare
6e60f1e to
44d3131
Compare
|
docs failures are unrelated #19134 |
|
Any subtlety wrt copy vs no copy? |
|
Well, the copy argument was only introduced in numpy 2.1 so the only consistent behavior we can get across all supported versions is the default |
|
I think if we can get approval on any of the set of 3, we can merge them all as all the diff are similar. Perhaps they could even have been one PR. |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
This is fine, but I would suggest to do just a single reshape and then assign to x, y[, z] - see in-line.
| result = np.dot(_create_matrix(angles[0], self.axes_order), inarr) | ||
| x, y, z = result[0], result[1], result[2] | ||
| x.shape = y.shape = z.shape = orig_shape | ||
| x, y, z = [np.reshape(a, orig_shape) for a in result] |
There was a problem hiding this comment.
I think this is better as x, y, z = np.reshape(result, (3,) + orig_shape)
| result = np.dot(cls._compute_matrix(angle), inarr) | ||
| x, y = result[0], result[1] | ||
| x.shape = y.shape = orig_shape | ||
| x = np.reshape(result[0], orig_shape) |
There was a problem hiding this comment.
Similarly, here I'd do x, y = np.reshape(result, (2,) + orig_shape)
There was a problem hiding this comment.
I tried a slight variant of this (np.reshape(result, (2, *shape))) which I think is exactly equivalent (?) to your suggestion, but it also breaks tests. Maybe I'm missing something ?
There was a problem hiding this comment.
Huh?? I'll need to check, that doesn't seem to make sense! (I assume you did do (2, *orig_shape)...)
There was a problem hiding this comment.
I assume you did do (2, *orig_shape)
yes
There was a problem hiding this comment.
for the record I can confirm that using your exact suggestion leads to the exact same problem (confirming that my variant is semantically identical).
| result = np.dot(augmented_matrix, inarr) | ||
| x, y = result[0], result[1] | ||
| x.shape = y.shape = shape | ||
| x = np.reshape(result[0], shape) |
|
Can we please prioritize this one? I see modeling code triggers the most downstream failures on my end. Thanks! |
44d3131 to
b74506d
Compare
|
I don't think there's anything left to do. As I said, Marten's suggestion isn't working (though I admit I don't know why). I rebased the branch so docs could rebuild cleanly, and anyone should be able to merge this without @pllim's super powers. |
|
I'd like to have a look myself, but also don't want to hold this up. How about if I don't say otherwise by end of today (EST), then just go ahead and merge? |
|
OK, I found it. In the projections, one is actually projecting, i.e., ignoring a third coordinate. Will push fix. |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Looks like all tests passed with my change, so let me approve.
One final thing: I went with your suggestion of np.reshape(result, (3, *shape)) instead of my original np.reshape(result, (3,)+shape), but it turns out this is the slower option:
In [6]: %timeit tuple((3,)+shape)
50.3 ns ± 0.925 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
In [7]: %timeit tuple((3, *shape))
105 ns ± 1.13 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
In the end I don't think it is worth changing, though, since the reshape is 1.5 us.
(Conversely, doing the reshape separately for each coordinate is quite a bit slower, so we are close to optimal now.)
pllim
left a comment
pllim
left a comment
There was a problem hiding this comment.
Thanks, all! I don't see modeling in the devdeps failures anymore, so let's get this in.
…#19150) Co-authored-by: P. L. Lim <[email protected]>
Description
ref #19126