Skip to content
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

provide compatibility with numpy 2.0 #38250

Merged
merged 3 commits into from
Jul 24, 2024
Merged

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented Jun 20, 2024

This will fix #38242

Basically, most of the problems are with numpy switching to a different way to display its data,
showing type. We either use a compatibility version, or add ellipses in doctests.

📝 Checklist

  • [x ] The title is concise and informative.
  • [x ] The description explains in detail what this PR is about.
  • [x ] I have linked a relevant issue or discussion.
  • [x ] I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Jun 20, 2024

Documentation preview for this PR (built with commit 515d20e; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@dimpase
Copy link
Member Author

dimpase commented Jun 21, 2024

Codecov complains about the lines numpy.set_printoptions(legacy="1.25") not covered by tests.
Not sure how to suppress these, it looks bogus to me.

@antonio-rojas
Copy link
Contributor

I'm still getting a test failure:

**********************************************************************
File "/usr/lib/python3.12/site-packages/sage/matrix/matrix1.pyx", line 709, in sage.matrix.matrix1.Matrix.numpy
Failed example:
    sorted(numpy.typecodes.items())                                       # needs numpy
Expected:
    [('All', '?bhilqpBHILQPefdgFDGSUVOMm'), ('AllFloat', 'efdgFDG'),
     ('AllInteger', 'bBhHiIlLqQpP'), ('Character', 'c'), ('Complex', 'FDG'),
     ('Datetime', 'Mm'), ('Float', 'efdg'), ('Integer', 'bhilqp'),
     ('UnsignedInteger', 'BHILQP')]
Got:
    [('All', '?bhilqnpBHILQNPefdgFDGSUVOMm'),
     ('AllFloat', 'efdgFDG'),
     ('AllInteger', 'bBhHiIlLqQnNpP'),
     ('Character', 'c'),
     ('Complex', 'FDG'),
     ('Datetime', 'Mm'),
     ('Float', 'efdg'),
     ('Integer', 'bhilqnp'),
     ('UnsignedInteger', 'BHILQNP')]
**********************************************************************

@dimpase
Copy link
Member Author

dimpase commented Jun 24, 2024

that's one I don't quite know how to fix.
I guess we don't actually need to check for values of these things, it is too verbose

@kiwifb
Copy link
Member

kiwifb commented Jun 24, 2024

Why is this even tested? I would understand if it was a numpy test, but a sage test?

@kiwifb
Copy link
Member

kiwifb commented Jun 24, 2024

Nitpicking, the string "# to ensure numpy 2.0 compatibility" is applied inconsistently. I suggest for it to be attached to the "if" statement line. I guess it will be clear if an import numpy statement had to be explicitly added.

@dimpase
Copy link
Member Author

dimpase commented Jun 24, 2024

Why is this even tested? I would understand if it was a numpy test, but a sage test?

it's not really a test, it's more like part of a guide. Useful, but testing all its output isn't so important. I added #random tag, and added one more example on how to show a part of it

@antonio-rojas
Copy link
Contributor

WFM now

@dimpase
Copy link
Member Author

dimpase commented Jun 25, 2024

Nitpicking, the string "# to ensure numpy 2.0 compatibility" is applied inconsistently. I suggest for it to be attached to the "if" statement line. I guess it will be clear if an import numpy statement had to be explicitly added.

I hope you're fine with undoing all that once numpy 2 becomes standard, see #38275

@dimpase
Copy link
Member Author

dimpase commented Jun 25, 2024

WFM now

thanks!

@kiwifb
Copy link
Member

kiwifb commented Jun 25, 2024

Nitpicking, the string "# to ensure numpy 2.0 compatibility" is applied inconsistently. I suggest for it to be attached to the "if" statement line. I guess it will be clear if an import numpy statement had to be explicitly added.

I hope you're fine with undoing all that once numpy 2 becomes standard, see #38275

Well, I did not say to remove it altogether. But I absolutely know what you are saying. In time those compatibility hacks will go.

@dimpase
Copy link
Member Author

dimpase commented Jun 29, 2024

it was just a trivial rebase

@tornaria
Copy link
Contributor

This PR works fine for me, except there is a change in src/sage/symbolic/expression.pyx that conflicts with #36641. Is there a chance you can remove that hunk here? Whoever is using sympy 1.13 will need to apply #36641 regardless.

@tornaria
Copy link
Contributor

Note: I've tested this PR using numpy 1.26 and using numpy 2.0 and it works fine on both cases. See void-linux/void-packages#49571 (comment).

@vbraun vbraun merged commit 0962e0b into sagemath:develop Jul 24, 2024
20 checks passed
@mkoeppe mkoeppe added this to the sage-10.5 milestone Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using numpy 2.0
7 participants