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

Fix matrix coercion with numpy 2.1 #38683

Merged
merged 3 commits into from
Sep 22, 2024
Merged

Conversation

antonio-rojas
Copy link
Contributor

numpy 2.1 requires a copy argument for numpy.array. Fixes test failures with numpy 2.1:

**********************************************************************
File "/usr/lib/python3.12/site-packages/sage/matrix/matrix1.pyx", line 723, in sage.matrix.matrix1.Matrix.numpy
Failed example:
    b = numpy.array(a); b
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 716, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 1146, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.matrix.matrix1.Matrix.numpy[9]>", line 1, in <module>
        b = numpy.array(a); b
            ^^^^^^^^^^^^^^
      File "sage/matrix/matrix1.pyx", line 673, in sage.matrix.matrix1.Matrix.numpy (build/cythonized/sage/matrix/matrix1.c:14076)
        def numpy(self, dtype=None):
    TypeError: numpy() got an unexpected keyword argument 'copy'
**********************************************************************
File "/usr/lib/python3.12/site-packages/sage/matrix/matrix1.pyx", line 727, in sage.matrix.matrix1.Matrix.numpy
Failed example:
    b.dtype
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 716, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 1146, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.matrix.matrix1.Matrix.numpy[10]>", line 1, in <module>
        b.dtype
        ^
    NameError: name 'b' is not defined
**********************************************************************
File "/usr/lib/python3.12/site-packages/sage/matrix/matrix1.pyx", line 730, in sage.matrix.matrix1.Matrix.numpy
Failed example:
    b.shape
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 716, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 1146, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.matrix.matrix1.Matrix.numpy[11]>", line 1, in <module>
        b.shape
        ^
    NameError: name 'b' is not defined
**********************************************************************

@kiwifb
Copy link
Member

kiwifb commented Sep 19, 2024

Is it compatible with numpy 2.0?

@antonio-rojas
Copy link
Contributor Author

Is it compatible with numpy 2.0?

works locally, I see some CI issues though

@kiwifb
Copy link
Member

kiwifb commented Sep 19, 2024

Should it be copy="copy"?

@kiwifb
Copy link
Member

kiwifb commented Sep 20, 2024

OK, I have read the manual and now I understand your patch. copy expect a boolean and the default is True. I think that should be the default for the numpy function rather than None (which I am guessing could be taken as False with some casting).

@antonio-rojas
Copy link
Contributor Author

antonio-rojas commented Sep 20, 2024

OK, I see the issue is incompatibility with numpy 1, which is still shipped in sage-the-distro. Let's try default to True. This parameter should only matter if you're calling numpy on a numpy object itself, which shouldn't be too common.

@antonio-rojas
Copy link
Contributor Author

Works locally now with 2.1, 2.0 and 1.26

Copy link

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

Copy link
Member

@kiwifb kiwifb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vbraun vbraun merged commit e9ded51 into sagemath:develop Sep 22, 2024
17 of 18 checks passed
@antonio-rojas antonio-rojas deleted the numpy-2.1 branch September 22, 2024 17:00
@user202729
Copy link
Contributor

user202729 commented Dec 18, 2024

Just come across this. While this fixes the tests, it isn't really correct:

  • It breaks Liskov substitution principle — Matrix_numpy_dense inherits from Matrix_dense, yet Matrix_numpy_dense doesn't allow passing copy=True to .numpy() method.
  • It doesn't really work correctly (see https://numpy.org/doc/stable/numpy_2_0_migration_guide.html#adapting-to-changes-in-the-copy-keyword and https://github.com/scipy/scipy/pull/20172/files for the proper fix)
  • It allows the user to pass copy=False which (by the interface contract) must return a view of the internal array, which is dangerous (depends specifically on the fact that the type is implemented with a numpy array). And this cannot be undone without a long deprecation period.
  • The copy=copy is in fact useless — self.list() is guaranteed to always return a copy, so even if copy=False is passed, it will make a copy anyway in numpy 1.26 and raise an error in numpy 2.0.

A fix is provided in #39152 . Maybe someone can take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants