Make matrix mod 2 conversion to numpy faster & some semantic fixes #39152
Conversation
user202729
force-pushed
the
faster-to-numpy
branch
from
5b316d0 to
97b8c50
Compare
user202729
force-pushed
the
faster-to-numpy
branch
from
97b8c50 to
bfb90b9
Compare
| @@ -303,7 +304,7 @@ def process_block(block, src_in_lines, file_optional_tags, venv_explainer=''): | |||
| got = re.sub(r'(doctest:warning).*^( *DeprecationWarning:)', | |||
| r'\1...\n\2', | |||
| got, 1, re.DOTALL | re.MULTILINE) | |||
| got = got.splitlines() # got can't be the empty string | |||
| got = textwrap.dedent(got).splitlines() # got can't be the empty string | |||
There was a problem hiding this comment.
Previously the fix made by the script would look like
sage: b = numpy.array(a); b
array([[ 0, 1, 2, 3],
[ 4, 5, 6, 7],
[ 8, 9, 10, 11]])
because each line is individually lstrip-ed. With the change it becomes
sage: b = numpy.array(a); b
array([[ 0, 1, 2, 3],
[ 4, 5, 6, 7],
[ 8, 9, 10, 11]])
|
Documentation preview for this PR (built with commit 6d4be29; changes) is ready! 🎉 |
user202729
force-pushed
the
faster-to-numpy
branch
from
271bb0e to
a1974a5
Compare
| """ | ||
| if copy is not _MISSING: | ||
| from sage.misc.superseded import deprecation | ||
| deprecation(39152, "passing copy argument to numpy() is deprecated") |
There was a problem hiding this comment.
I decide to deprecate this feature because:
copy=Falsewas never supported in the first place (it always copies)copy=Falseimplicitly copies is incompatible with numpy 2.0 interface wherenp.array(..., copy=False)raisesValueErrorif a copy is madecopy=*doesn't work in numpy-based matrices (even after Fix matrix coercion with numpy 2.1 #38683 )- it seems dangerous to expose the internal array (which will change on mutation on the original object, which requires implementation to use the exact dtype otherwise user visible change will be seen)
| 20000 | ||
| """ | ||
| from ..modules.numpy_util import mzd_matrix_to_numpy | ||
| return mzd_matrix_to_numpy(<uintptr_t>self._entries, dtype) |
There was a problem hiding this comment.
This method was the original plan (it overrides numpy() method of parent to provide a fast path)
| return self._matrix_numpy.copy() | ||
| else: | ||
| return Matrix_dense.numpy(self, dtype=dtype) | ||
| return np.array(self._matrix_numpy, dtype=dtype) |
There was a problem hiding this comment.
Simplification.
Also the old code made a call to generic Matrix_dense.numpy(self, dtype=dtype) method which is obviously much slower than this.
+ the old code documentation is in fact incorrect because the __array__ method of Matrix_numpy_dense class is not overridden, so the method is not called inside np.array(...). Of course it works because of the generic (slow) implementation.
There was a problem hiding this comment.
what's wrong with flintlib/flint#2027 so that it "prevents testing" ?
There was a problem hiding this comment.
Oh, nothing. I just mean that I need a version of flint after that pull request to allow testing, and I haven't gotten around to figure out how to install it from source yet (since latest version on conda-forge didn't have that pull request merged)
There was a problem hiding this comment.
I think in Sage we have a workaround (some macro magic) for this Flint issue installed. (Flint still hasn't released a version with this fix merged)
user202729
changed the title
As in the title. Plus a few minor changes as needed.
Reuses
numpy_utilmodule from #38834 for the utility function…in retrospect it might have been placed in the wrong place. (?)
📝 Checklist
⌛ Dependencies
Haven't been able to test on numpy 2.1 because of
https://github.com/flintlib/flint/pull/2027