Replace deprecated setting of shape and dtype on numpy arrays #18562
Conversation
|
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.
|
eendebakpt
changed the title
docs/changes/18562.other.rst
Outdated
There was a problem hiding this comment.
Since this change is invisible to users, we do not need a change log. Please delete this file and squash it out in your commit. Thanks!
|
Thanks! I am leaning against not backporting since the upstream PR is still open and maybe by the time the warning hits us, v7.1.x would be retired. |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
@eendebakpt - thanks so much for taking this on! I think most changes here are straightforward and none are in places where a few us is going to matter. So, I'm happy to just approve for the various subpackages.
But I'd like to do uncertainty separately, since speed there is more important, and also I think that the two changes you have here are not the only ones - there is a specific dtype setter that I at least would want to think a bit more about. It may be that it is reasonable here to use the private methods that still allow setting.
Anyway, bottom line would be to try to only do sub-packages here in which this PR makes them fully compatible with the future numpy 2.4!
With that request to remove uncertainty for now, I guess you might as well address my minute comments, of using .reshape() more directly (see in-line comments).
| def test_groups_hdu_data(self): | ||
| imdata = np.arange(100.0) | ||
| imdata.shape = (10, 1, 1, 2, 5) | ||
| imdata = imdata.reshape((10, 1, 1, 2, 5)) |
There was a problem hiding this comment.
Nitpick: I'd add the .reshape to the definition, like you do below.
| if shape is not None: | ||
| a.shape = shape | ||
| b.shape = shape | ||
| a = a.reshape(shape) |
There was a problem hiding this comment.
This routine can be fairly easily rewritten to not do the ravel, reshape at all. I can push a commit to your branch, though perhaps it is better I do it in follow-up, so we can just get this in first, since what you have here is the obvious solution of just replacing setting .shape with a .reshape().
There was a problem hiding this comment.
Yes, I prefer a followup pR.
| self.n1 = rsn.standard_normal(self.x1.size) * 0.1 | ||
| self.n2 = rsn.standard_normal(self.x2.size) | ||
| self.n2.shape = self.x2.shape | ||
| self.n2 = self.n2.reshape(self.x2.shape) |
There was a problem hiding this comment.
Maybe do the reshape on the definition.
astropy/uncertainty/core.py
Outdated
| structured = np.asarray(DummyArray(interface, base=samples)) | ||
| # Set our new structured dtype. | ||
| structured.dtype = new_dtype | ||
| structured = structured.view(new_dtype) |
There was a problem hiding this comment.
This is probably not quite it for uncertainty, correct? If so, I'd prefer to do uncertainty separately -- maybe for that one it is worth using the private methods.
There was a problem hiding this comment.
I reverted the changes for now just to be sure.
So far I picked the low-hanging fruit, but indeed it might be good to organize per sub-package. In the |
|
I think it would help review indeed to split out sub-packages that are more tricky, ensuring that with whatever is merged, a sub-package is ready to face the immutable shape/dtype/stride world... |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
OK, even if this doesn't quite complete fits, I think it is a good start, and all the changes are good even if numpy never were to deprecate settings .shape and .dtype. Thanks, @eendebakpt!
3 participants
Description
Replaces direct setting of the
shapeanddtypeparameters of numpy arrays.In this PR we handle only part of the cases. In followup PRs the remaining cases can be handled.
Fixes (partly) #18561