-
Notifications
You must be signed in to change notification settings - Fork 97
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.
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
Numpy 2.0 #689
base: main
Are you sure you want to change the base?
Conversation
28bb96a
to
2808634
Compare
Removed the custom Complex C types... I have no context as to why they were necessary, so that definitely needs careful review. This is code that seems to have been there mostly from the beginning. Edit seems to be needed to define operations like |
d83b37d
to
eca375b
Compare
Also removes special case for old unsupported numpy 1.16
In what context? If it is in array indexing, then this syntax was only added on Python 3.11 (I found out recently about this). |
No, this is C(++) code, multiplication of complex scalars. Maybe @michaelosthege finds this interesting xD? These are the breaking changes: https://numpy.org/devdocs/numpy_2_0_migration_guide.html#complex-types-underlying-type-changes |
This will help with publishing a repodata patch for conda-forge as per <pymc-devs#688 (comment)>. This will no longer be necessary when we are finished with <pymc-devs#689>.
This will help with publishing a repodata patch for conda-forge as per <#688 (comment)>. This will no longer be necessary when we are finished with <#689>.
Friendly note that it will be necessary to revert 1235d08 on the next rebase in order to unpin |
There's some esoteric C++ code for complex variables that I didn't want to dig too much into |
Is this PR stalled? The numpy release notes seem to suggest what changes need to be made (replace |
There's something a bit more complex. We are using c++ to overload operators of "scalar" complex numbers. This goes beyond the standard use of numpy and their advice is not sufficient here. |
Okay, it's possible I'm misunderstanding. I thought that code is defining a struct (e.g. It does seem a bit annoying in that I'm not certain I can fix it, but I'd be interested in trying. (Although if the above is completely off base, please let me know... in that case, I certainly don't understand the problem.) |
@brendan-m-murphy I think you're on the right track. The part where we overload operators was where I stopped at. Compiler wasn't happy with my changes to use the new real/complex accessors here https://github.com/pymc-devs/pytensor/blob/main/pytensor%2Fscalar%2Fbasic.py#L527-L538 Feel free to take a stab at it. You can push to this branch or if you don't have permissions start a new PR either on top of my branch or directly to main |
You'll have to revert "Remove custom Complex type", that was just to investigate what part of the codebase actually depended on it |
Maybe this table is helpful? Also there is this note in the NumPy 2.0.0 release notes and this one on NumPy 1 & 2 compatibility. Namely NumPy 2 is just using C99's complex types |
Yes, I think using C99 types is the reason for moving to |
Just an update: I've updated the code for There are still some failing tests related to the change to complex numbers in Numpy in An issue I had with the The The updates I made will work for 32 and 64 bit machines, assuming that long double on the 32-bit machine is actually 64-bit. To get more options, I suppose we could copy numpy's set up of defining everything in terms of float, double, long double, then having set-up specific aliases for types in terms of the number of bits. |
I made some small changes to handle the typedefs for npy_cdouble etc. a bit smoother. I think those changes are pretty much ready to merge into this branch. There are a lot of mypy issues, but I suspect they're not related to my changes, which were mostly to the C code. I'll try to tidy it up and then request a review. I think there are serious problems with the There are some suggestions on that PR and this Aesara issue: aesara-devs/aesara#1506 I could try to work on this, but I suspect it would take me longer than the complex type updates. |
Thanks @brendan-m-murphy. For context the It is the only form of AdvancedIndexing implemented in C. Looking at the C implementation, it seems like quite a mess, and possibly overly complicated/inefficient. Like why is the Anyway, since we are trying to part ways with the C backend, I think we can make this operator C-implementation conditional on the numpy version that is installed? If we raise NotImplementedError from Otherwise if someone feels like doing C-implementation, help is welcome. I think the only issue is replacing the MapIter logic |
@brendan-m-murphy regarding complex32, I also don't recall it being a thing these days. Just drop it |
Last thing @brendan-m-murphy if possible, it would be great if the complex stuff worked both in numpy 1.x and 2.x. Maybe it does, just stating in case it was never brought up. |
It doesn't currently, but I think it's an easy change (I'm pretty sure the numpy release notes say how). I suppose I could make a PR against main then (I'd probably do a new branch for that with just the complex variable changes). Also, I could make the updates for complex variables to |
We can tweak the test CI to run with both versions of numpy |
So if the C code isn't available, the PerformLinker would be used for an Apply node, even if the mode says to use the CLinker? This (rather old) commit seems to already implement a version of the fancy indexing increment using numpy for the "perform" function in : c60bd3b (all the C code is missing here, but that was moved into the class a few years ago). |
The linker doesn't change, but the default already handles mixing C and Python implementations. When an Op raises NotImplementedError for C code it builds a thunk around the python perform method instead. |
Oh okay, so raising the NotImplementedError would use I guess there could be a performance hit, but it sounds like the numpy team are working on improving I'll try to get my PR in soon. I had to move to other things at work, but should be able to finish it in an evening or two. |
The slowdown seems worthwhile specially if |
Just an update: raising a not implemented error for the advanced inc subtensor when using numpy 2.0 removes the errors around the MapIter (as expected), but it revealed more errors related to the complex arithmetic. I haven't had a lot of time to look into these new errors, but I'll try to find out more this week. |
Thanks 🙏 Hopefully we're getting there! |
Some of the tests in This is for The error comes from trying to create a TensorConstant from 1e-6. Ultimately, the check Locally (on a macbook m2), this check fails with numpy 2.0 and numpy 1.26. I guess I don't see why this error shouldn't occur, since to get to this point in I haven't tested that the updates to complex numbers work for numpy 1.26 yet, but with numpy 2.0, basic tensor operations work with complex types now. The tests with the latest updates are here: https://github.com/brendan-m-murphy/pytensor/actions/runs/10144114724/job/28047006005?pr=2 I'll update the workflow to run against numpy 1.26 as well. |
That failure doesn't seem too worrisome, I guess it has to do with change in behavior of numpy regarding casting. The float32 tests are always a bit of a pain around here. Feel free to cast explicitly to make the test pass or tweak the test parameters so we're not testing such large values. This is completely unrelated to the main goal of those tests |
I'm still working through the failing tests. For For For the random variable tests, I had to replaced |
In |
Maybe the answer to this last problem and the overflow problem are in NEP 50: https://numpy.org/neps/nep-0050-scalar-promotion.html One other part of the tests with many failures is the rewriting tests for ops involving random variables (e.g. |
Do you have a branch with the most recent changes? We can start triaging the failing tests and see what's problematic vs inconsequential |
Here is the most recent version: brendan-m-murphy#2 |
I'll make a list of failing tests here (as I look into them):
|
Description
Ruff suggested a few things to change but I cannot test because I install both
numpy=2.0.0.rc1
andnumba
(any version including the most recent0.59.1
) (incompatibilities in python abi).Related Issue
Checklist
Type of change