search

GalSim-developers / GalSim

The modular galaxy image simulation toolkit. Documentation:
http://galsim-developers.github.io/GalSim/
Other
223 stars 105 forks source link

Fixes for numpy 2 #1293

Closed erykoff closed 2 months ago

erykoff commented 3 months ago

This should work with numpy 2 now (tested locally with numpy 2.0.0rc2 and python=3.12). I based this off the 2.5 release branch, I don't know if that's the right thing to do.

One test -- checking the wfirst deprecation -- isn't working and I don't understand that or why it should be related to numpy2. Maybe a py312 thing?

rmjarvis commented 3 months ago

I had already started this too, and figured I'd keep going before looking at how you did everything. cf. https://github.com/GalSim-developers/GalSim/compare/releases/2.5...numpy2

Here are the salient differences as I see them:

  1. You should cherry-pick 09b8ddd5 to get the CI to test with numpy 2.0 with Python 3.12. At least until 2.0 becomes standard, we'll want to test with both numpy 1 and 2.
  2. I think np.asarray(a) is a better change for most of the np.array(a, copy=False) cases. There was one case in table.py, where we do something slightly different, so that requires a version check, but most things don't.
  3. I think the OverflowErrors are actually a numpy bug, so I don't particularly want to add a bunch of cruft to GalSim to work around it. I'd rather just change the tests. Honestly, I think in real world cases, if a user got the OverflowError, it's more likely a user error, so we probably want to just let that be.
  4. Finally, to address the change in np.testing.assert_array_equal behavior, I think it's easier to just let the images for those tests use dtype=float, rather than add explicit casts in the assert call. Feels a bit less obfuscating to me.
  5. np.trapz is deprecated now. We don't use it much (since our own version is faster), but there were a few cases, which I switched over to our implementation. cf. 84a9495cf060696053ca7af3bb9293e5c76f3016
erykoff commented 3 months ago

@rmjarvis Thanks for your comments and commits to cherry-pick. Re (2): in numpy 1.x, np.array() will by default do a copy unless copy=False; in 2.x np.array() will only do a copy if necessary by default. (Note that this is different behavior than astype() which would only do a copy if necessary by default in 1.x and 2.x). So if we want to minimize copies in 1.x (which we do!) we need the full shim wherever main uses copy=False. Re (3) I'm not sure what I should change in the tests (though I do see your point that this may be a numpy bug).

rmjarvis commented 3 months ago

So if we want to minimize copies in 1.x (which we do!) we need the full shim wherever main uses copy=False.

I don't think that's true. At least for most uses in GalSim. (I.e. aside from the two in table.py.) np.asarray(a, dtype) does the same thing in both 1.x and 2.0 as np.array(a, dtype, copy=False) does in 1.x and what np.array(a, dtype, copy=None) does in 2.0.

rmjarvis commented 3 months ago

Re (3) I'm not sure what I should change in the tests (though I do see your point that this may be a numpy bug).

My solution was to change -1 to np.int64(-1) in __neg__, and in the places where we add a negative value, cast it to np.int64. cf. 1a9756077

erykoff commented 3 months ago

Ah, okay. Replacing np.array() with np.asarray() would do the trick without the shim in many cases. I missed the subtle replacement of array() with asarray() in your comment and why are there both of these anyway? (Rhetorical question)

rmjarvis commented 3 months ago

Apparently that OverFlowError is an intended anti-feature. cf. https://github.com/numpy/numpy/issues/26483#issuecomment-2123230081 But changing to use numpy int types in such cases seems like the right thing to do to work around it.

rmjarvis commented 2 months ago

Closing in lieu of #1297. Thanks Eli for the first pass and discussion about the various options to make the code compatible with numpy 2.0.