Replace "... is SR" by isinstance(..., sage.rings.abc.SymbolicRing) to handle symbolic subrings

Question

Replace "... is SR" by isinstance(..., sage.rings.abc.SymbolicRing) to handle symbolic subrings

Closed this issue 4 years ago · 26 comments

There are a number of tests ... is SR in the Sage library, as found by git grep 'is SR' src/sage/

These tests ignore symbolic subrings, as constructed by SR.subring(...).

All or most of these uses should be changed to isinstance(..., sage.rings.abc.SymbolicRing) or isinstance(..., sage.symbolic.ring.SymbolicRing) (if the module already imports other things from sage.symbolic)

In addition to handling the case of subrings, this will help with modularization (#32601).

We leave a few uses of is SR to follow-up tickets #31988, #31999.

CC: @dkrenn @egourgoulhon @orlitzky

Component: symbolics

Author: Matthias Koeppe

Branch/Commit: ff83c4c

Reviewer: Michael Orlitzky

Issue created by migration from https://trac.sagemath.org/ticket/32724

Answer 1 · 2021-10-20T19:53:52.000Z

Branch: u/mkoeppe/replace______is_sr__by_isinstance______sage_rings_abc_symbolicring__to_handle_symbolic_subrings

Answer 2 · 2021-10-20T19:54:30.000Z

Commit: 45aab88

Answer 3 · 2021-10-20T19:54:30.000Z

comment:2

here's one such change, cherry-picked from #32601

New commits:

`45aab88`	`src/sage/rings/polynomial/polynomial_element.pyx: Use isinstance(..., sage.rings.abc.SymbolicRing) instead of ... is SR`

Answer 4 · 2021-10-20T19:54:30.000Z

Author: Matthias Koeppe, ...

Answer 5 · 2021-11-01T03:10:00.000Z

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

`da82ac5`	`src/sage/rings/polynomial/polynomial_element.pyx: Use isinstance(..., sage.rings.abc.SymbolicRing) instead of ... is SR`
`3c821bd`	`sage.functions.other._eval_floor_ceil: Handle elements of symbolic subrings like elements of SR`

Answer 6 · 2021-11-01T03:10:00.000Z

Changed commit from 45aab88 to 3c821bd

Answer 7 · 2021-11-01T03:18:56.000Z

Changed commit from 3c821bd to f1c9258

Answer 8 · 2021-11-01T03:18:56.000Z

Branch pushed to git repo; I updated commit sha1. New commits:

`f1c9258`	`src/sage/combinat/q_analogues.py: Handle symbolic subrings like SR`

Answer 9 · 2021-11-01T03:59:30.000Z

Branch pushed to git repo; I updated commit sha1. New commits:

`f98855b`	`src/sage/dynamics/arithmetic_dynamics/projective_ds.py: Use sage.rings.abc.SymbolicRing instead of 'is SR'`
`7f1500e`	`src/sage/geometry/polyhedron/parent.py: For backend='normaliz', accept subrings of SR`

Answer 10 · 2021-11-01T03:59:30.000Z

Changed commit from f1c9258 to 7f1500e

Answer 11 · 2021-11-01T04:09:05.000Z

Branch pushed to git repo; I updated commit sha1. New commits:

`76e0962`	`[Diff]ScalarFieldAlgebra._coerce_map_from_: Also coerce from subrings of SR`

Answer 12 · 2021-11-01T04:09:05.000Z

Changed commit from 7f1500e to 76e0962

Answer 13 · 2021-11-01T04:14:00.000Z

Description changed:

--- 
+++ 
@@ -2,7 +2,7 @@
 
 These tests ignore symbolic subrings, as constructed by `SR.subring(...)`.
 
-All or most of these uses should be changed to `isinstance(..., sage.rings.abc.SymbolicRing)`.
+All or most of these uses should be changed to `isinstance(..., sage.rings.abc.SymbolicRing)` or `isinstance(..., sage.symbolic.ring.SymbolicRing)` (if the module already imports other things from `sage.symbolic`)
 
 In addition to handling the case of subrings, this will help with modularization (#32601).

Answer 14 · 2021-11-01T05:33:23.000Z

Changed commit from 76e0962 to 5fca7fc

Answer 15 · 2021-11-01T05:33:23.000Z

Branch pushed to git repo; I updated commit sha1. New commits:

`b7470d1`	`Matrix.is_{positive_operator,cross_positive,lyapunov_like}_on: Handle symbolic subrings like SR`
`5fca7fc`	`continued_fraction: Handle symbolic subrings like SR`

Answer 16 · 2021-11-01T05:46:20.000Z

Changed commit from 5fca7fc to 038eb9f

Answer 17 · 2021-11-01T05:46:20.000Z

Branch pushed to git repo; I updated commit sha1. New commits:

`038eb9f`	`src/sage/symbolic/pynac_impl.pxi (py_is_integer, py_is_exact): Handle symbolic subrings like SR`

Answer 18 · 2021-11-01T06:00:11.000Z

Changed commit from 038eb9f to ff83c4c

Answer 19 · 2021-11-01T06:00:11.000Z

Branch pushed to git repo; I updated commit sha1. New commits:

`ff83c4c`	`sage.repl.ipython_kernel (widget_from_tuple, slider): Handle symbolic subrings like SR`

Answer 20 · 2021-11-01T06:03:26.000Z

Changed dependencies from #32665 to none

Answer 21 · 2021-11-01T06:03:26.000Z

Changed author from Matthias Koeppe, ... to Matthias Koeppe

Answer 22 · 2021-11-01T06:03:26.000Z

Description changed:

--- 
+++ 
@@ -6,3 +6,4 @@
 
 In addition to handling the case of subrings, this will help with modularization (#32601).
 
+We leave a few uses of `is SR` to follow-up tickets #31988, #31999.

Answer 23 · 2021-11-06T16:46:28.000Z

comment:13

Now that we have a passing patchbot, this LGTM. I think "the symbolic ring (or a symbolic subring)" is awkward to repeat so many times and would be better phrased "a symbolic ring," but it's not important.

Answer 24 · 2021-11-06T16:46:28.000Z

Reviewer: Michael Orlitzky

Answer 25 · 2021-11-06T17:35:08.000Z

comment:14

Thank you!

I used this phrasing because I didn't want to think about the status of callable symbolic rings in this ticket.

Answer 26 · 2021-11-12T21:27:36.000Z

Changed branch from u/mkoeppe/replace______is_sr__by_isinstance______sage_rings_abc_symbolicring__to_handle_symbolic_subrings to ff83c4c