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.

Sign up for GitHub

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

Jump to bottom

sage.matrix: Modularization fixes after #33159 #33392

Closed
mkoeppe opened this issue Feb 21, 2022 · 11 comments
Closed

sage.matrix: Modularization fixes after #33159 #33392

mkoeppe opened this issue Feb 21, 2022 · 11 comments
Labels
p: major / 3 t: enhancement t: refactoring
Milestone
sage-9.6

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 21, 2022

We add # optional - sage.symbolic doctest tags
after #33159.

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: 2f8a489

Reviewer: Michael Orlitzky

Issue created by migration from https://trac.sagemath.org/ticket/33392
@mkoeppe mkoeppe added this to the sage-9.6 milestone Feb 21, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 21, 2022

Branch: u/mkoeppe/sage_matrix__modularization_fixes_after__33159

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 21, 2022

Commit: 2f8a489

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 21, 2022

New commits:

b28224bsrc/sage/matrix/matrix2.pyx: Add # optional - sage.symbolic
2f8a489src/sage/matrix/matrix2.pyx: Use sage.rings.abc.SymbolicRing for isinstance testing

@mwageringel
Copy link

comment:3

How can I test this? And how do we prevent this from happening again?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 21, 2022

comment:4

I noticed it by running #32432 (see test instructions on the ticket description).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 21, 2022

comment:5

Replying to @mwageringel:

how do we prevent this from happening again?

When #32432 or #32601 are merged, we can add it to our automated tests.

@slel

This comment has been minimized.

Sign in to view
@orlitzky
Copy link
Contributor

Reviewer: Michael Orlitzky

@orlitzky
Copy link
Contributor

comment:7

Well, it can't hurt. Except for that one isinstance() which LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 23, 2022

comment:8

Thanks!

@vbraun
Copy link
Member

vbraun commented Feb 27, 2022

Changed branch from u/mkoeppe/sage_matrix__modularization_fixes_after__33159 to 2f8a489

@vbraun vbraun closed this as completed in 0fed9a1 Feb 27, 2022
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 12, 2023
@mkoeppe
Merge sagemath#33392
4e477f1
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 17, 2023
@mkoeppe
Merge sagemath#33392
50f31f5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
p: major / 3 t: enhancement t: refactoring
Projects
None yet
Milestone
sage-9.6
Development

No branches or pull requests
5 participants
@vbraun @orlitzky @slel @mkoeppe @mwageringel