Remove or tag uses of SR and symbolic functions in combinat, categories, etc.

Question

Remove or tag uses of SR and symbolic functions in combinat, categories, etc.

Closed this issue 4 years ago · 53 comments

Follow-up on #32411.

Use of floor/ceil from sage.functions can be avoided.

Modules that only need symbolics in some methods should avoid module-level imports from sage.functions, sage.symbolic, sage.calculus.

Doctests that need these modules should be marked # optional - sage.symbolic.

This can be tested using #32601 (sagemath-standard-no-symbolics)

CC: @tscrim

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: 4a1b489

Reviewer: Samuel Lelièvre

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

Answer 1 · 2022-02-27T22:58:22.000Z

Branch: u/mkoeppe/remove_more_unnecessary_uses_of_sr_and_symbolic_functions_in_sage_combinat

Answer 2 · 2022-02-27T23:02:15.000Z

Commit: 04855a1

Answer 3 · 2022-02-27T23:02:15.000Z

New commits:

`6bd6903`	`PowerSeriesRing_generic._element_constructor_: Only import sage.symbolic.expression.SymbolicSeries after isinstance test with ABC Expression`
`9730b10`	`src/sage/categories/metric_spaces.py: Mark doctests using HyperbolicPlane # optional - sage.symbolic`
`dca7113`	`In doctests, replace floor(.../...) by //`
`44ac5ab`	`src/sage/categories/vector_bundles.py: Mark doctests # optional - sage.symbolic`
`71303dd`	`src/sage/matrix/operation_table.py: Use math.log, log10 insteadd of importing from sage.misc.functional`
`04855a1`	`src/sage/groups/matrix_gps/coxeter_group.py: Do not use sage.symbolic for easy cases of cos(pi/x)`

Answer 4 · 2022-02-27T23:02:15.000Z

Description changed:

--- 
+++ 
@@ -2,31 +2,9 @@
 
 Use of `floor`/`ceil` from `sage.functions` can be avoided.
 
-Modules that only need symbolics in some methods should avoid module-level imports from `sage.functions`, `sage.symbolic`, `sage.calculus`
+Modules that only need symbolics in some methods should avoid module-level imports from `sage.functions`, `sage.symbolic`, `sage.calculus`.
 
-```
-src/sage/categories/loop_crystals.py:from sage.functions.other import ceil
-src/sage/combinat/binary_recurrence_sequences.py:from sage.functions.log import log
-src/sage/combinat/binary_recurrence_sequences.py:from sage.functions.other import sqrt
-src/sage/combinat/crystals/littelmann_path.py:from sage.functions.other import floor
-src/sage/combinat/crystals/tensor_product.py:from sage.functions.other import floor, ceil
-src/sage/combinat/crystals/tensor_product_element.pyx:from sage.functions.other import ceil
-src/sage/combinat/diagram_algebras.py:from sage.functions.other import floor, ceil
-src/sage/combinat/finite_state_machine.py:from sage.calculus.var import var
-src/sage/combinat/finite_state_machine.py:from sage.functions.trig import atan2
-src/sage/combinat/k_tableau.py:from sage.functions.generalized import sgn
-src/sage/combinat/parallelogram_polyomino.py:from sage.functions.trig import cos, sin
-src/sage/combinat/parallelogram_polyomino.py:from sage.functions.other import sqrt
-src/sage/combinat/partition.py:from sage.symbolic.ring import var
-src/sage/combinat/partition_algebra.py:from sage.functions.all import ceil
-src/sage/combinat/root_system/coxeter_type.py:from sage.symbolic.ring import SR
-src/sage/combinat/sine_gordon.py:from sage.functions.trig import cos, sin
-src/sage/combinat/sine_gordon.py:from sage.symbolic.constants import pi, I
-src/sage/combinat/sine_gordon.py:from sage.functions.log import exp
-src/sage/combinat/sine_gordon.py:from sage.functions.other import ceil
-src/sage/combinat/sine_gordon.py:from sage.symbolic.ring import SR
-src/sage/combinat/sine_gordon.py:from sage.functions.other import real_part, imag_part
-src/sage/combinat/sloane_functions.py:from sage.functions.all import prime_pi
-src/sage/combinat/symmetric_group_representations.py:from sage.symbolic.ring import SR
-src/sage/combinat/symmetric_group_representations.py:from sage.functions.all import sqrt
-```
+Doctests that need these modules should be marked `# optional - sage.symbolic`.
+
+This can be tested using #32601 (**sagemath-standard-no-symbolics**)
+

Answer 5 · 2022-02-27T23:02:45.000Z

Author: Matthias Koeppe

Answer 6 · 2022-02-27T23:13:05.000Z

Changed commit from 04855a1 to ecad015

Answer 7 · 2022-02-27T23:13:05.000Z

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

`ecad015`	`src/sage/groups/matrix_gps/coxeter_group.py: Do not use sage.symbolic for easy cases of cos(pi/x) (fixup)`

Answer 8 · 2022-02-28T00:45:40.000Z

Changed commit from ecad015 to 2511e3e

Answer 9 · 2022-02-28T00:45:40.000Z

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

`5d2cc6b`	`PowerSeriesRing_generic._element_constructor_: Only import sage.symbolic.expression.SymbolicSeries after isinstance test with ABC Expression`
`dc8cc8c`	`src/sage/categories/metric_spaces.py: Mark doctests using HyperbolicPlane # optional - sage.symbolic`
`9ca27e7`	`In doctests, replace floor(.../...) by //`
`444b659`	`src/sage/categories/vector_bundles.py: Mark doctests # optional - sage.symbolic`
`63b0ad8`	`src/sage/matrix/operation_table.py: Use math.log, log10 insteadd of importing from sage.misc.functional`
`13c5092`	`src/sage/groups/matrix_gps/coxeter_group.py: Do not use sage.symbolic for easy cases of cos(pi/x)`
`2511e3e`	`src/sage/groups/matrix_gps/coxeter_group.py: Do not use sage.symbolic for easy cases of cos(pi/x) (fixup)`

Answer 10 · 2022-02-28T01:06:54.000Z

Changed commit from 2511e3e to ea8d379

Answer 11 · 2022-02-28T01:06:54.000Z

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

`e3b36fa`	`src/sage/categories/algebra_functor.py: Make doctests using sqrt # optional - sage.symbolic`
`ea8d379`	`src/sage/categories/rings.py: Mark doctests using sqrt # optional - sage.symbolic`

Answer 12 · 2022-02-28T01:31:26.000Z

Changed commit from ea8d379 to 9cae6cb

Answer 13 · 2022-02-28T01:31:26.000Z

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

`c7bd9ad`	`src/sage/arith/multi_modular.pyx: Use primecountpy.primecount directly instead of going through sage.functions`
`9cae6cb`	`src/sage/groups/semimonomial_transformations/semimonomial_transformation_group.py: Import factorial from sage.arith.misc`

Answer 14 · 2022-02-28T02:24:48.000Z

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

`994f47b`	`src/sage/categories/number_fields.py: Remove abuse of predefined x in doctests`
`4a1fc16`	`src/sage/structure/element.pyx: Remove abuse of predefined x in doctests`
`c6714aa`	`src/sage/categories/coxeter_groups.py: Mark a doctest # optional - sage.symbolic`
`30fa675`	`src/sage/categories/pushout.py: Remove abuse of predefined x in doctests`
`3cd7195`	`src/sage/categories/modules_with_basis.py: In doctest, import factorial from sage.arith.misc`

Answer 15 · 2022-02-28T02:24:48.000Z

Changed commit from 9cae6cb to 3cd7195

Answer 16 · 2022-02-28T03:06:29.000Z

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

`2445b36`	`src/sage/categories/chain_complexes.py: Mark doctests using manifolds # optional - sage.symbolic`

Answer 17 · 2022-02-28T03:06:29.000Z

Changed commit from 3cd7195 to 2445b36

Answer 18 · 2022-02-28T03:12:57.000Z

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

`fe0c388`	`src/sage/categories/lie_algebras.py: Mark doctests using manifolds # optional - sage.symbolic`

Answer 19 · 2022-02-28T03:12:57.000Z

Changed commit from 2445b36 to fe0c388

Answer 20 · 2022-02-28T03:15:58.000Z

Changed commit from fe0c388 to e197558

Answer 21 · 2022-02-28T03:15:58.000Z

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

`e197558`	`src/sage/categories/finite_dimensional_nilpotent_lie_algebras_with_basis.py: Mark doctests using manifolds # optional - sage.symbolic`

Answer 22 · 2022-02-28T03:19:34.000Z

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

`3ede66a`	`src/sage/categories/finite_permutation_groups.py: Mark a doctest # optional - sage.symbolic`
`caf233e`	`src/sage/categories/metric_spaces.py: Mark a doctest # optional - sage.symbolic`

Answer 23 · 2022-02-28T03:19:34.000Z

Changed commit from e197558 to caf233e

Answer 24 · 2022-02-28T03:27:27.000Z

Changed commit from caf233e to 486fafb

Answer 25 · 2022-02-28T03:27:27.000Z

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

`486fafb`	`src/sage/categories/pushout.py: Mark doctests # optional - sage.symbolic`

Answer 26 · 2022-02-28T03:57:34.000Z

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

`adab09f`	`src/sage/categories/map.pyx: Mark doctests # optional - sage.symbolic`

Answer 27 · 2022-02-28T03:57:34.000Z

Changed commit from 486fafb to adab09f

Answer 28 · 2022-02-28T05:43:36.000Z

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

`016cd5b`	`src/sage/categories/finite_posets.py: In doctest, remove use of floor`
`dd9dccd`	`src/sage/categories/primer.py: Mark doctests # optional - sage.symbolic`
`b0fe2b7`	`src/sage/categories/homset.py: Remove abuse of predefined x in doctests`
`dbecc5f`	`src/sage/categories/homset.py: Mark doctests # optional - sage.symbolic`
`e177b45`	`src/sage/categories/number_fields.py: Mark doctests # optional - sage.symbolic`

Answer 29 · 2022-02-28T05:43:36.000Z

Changed commit from adab09f to e177b45

Answer 30 · 2022-02-28T21:58:26.000Z

comment:17

Some more parts can be made symbolics-free,
though sometimes it makes things convoluted.

File src/sage/categories/algebra_functor.py

We can bypass using sqrt(5) to construct ZZ[sqrt(5)]:

    sage: G = GL(2, 7)
    sage: K = QuadraticField(5, 'srqt5')
    sage: Zr5 = K.order(K.gen())  # This is ZZ[sqrt(5)]
    sage: OG = GroupAlgebra(G, Zr5)
    sage: OG(2)
    2*[1 0]
    [0 1]
    sage: OG(G(2))
    [2 0]
    [0 2]
 
    sage: OG(FormalSum([ (1, G(2)), (2, RR(0.77)) ]) )
    Traceback (most recent call last):
    ...
    TypeError: Attempt to coerce non-integral RealNumber to Integer
    sage: OG(OG.base_ring().basis()[1])
    sqrt5*[1 0]
    [0 1]

Keeping the clearer ZZ[sqrt(5)] and adding
# optional - sage.symbolic makes sense too.

I wish sqrt(5) gave an element in a number field
or AA or QQbar, not in the symbolic ring...

File src/sage/categories/coxeter_groups.py

Using a polynomial variable can bypass symbolics here:

                 sage: W = CoxeterGroups().example()
-                sage: sum((x^w.length()) for w in W) - expand(prod(sum(x^i for i in range(j+1)) for j in range(4))) # This is scandalously slow!!!  # optional - sage.symbolic
+                sage: x = polygen(ZZ)
+                sage: s = sum(x^w.length() for w in W)
+                sage: p = prod(sum(x^i for i in range(j)) for j in range(1, 5))
+                sage: s - p
                 0

The 2009 "scandalously slow" comment can probably go?

File src/sage/categories/pushout.py

Using sqrt(3.) and (2.)^(1/3) makes this symbolics-free:

-            sage: K.<a> = NumberField(x^3-2, embedding=CDF(1/2*I*2^(1/3)*sqrt(3) - 1/2*2^(1/3)))
+            sage: cbrt2 = CDF(2)^(1/3)
+            sage: zeta3 = CDF(-1/2, sqrt(3.)/2)
+            sage: K.<a> = NumberField(x^3 - 2, embedding=cbrt2 * zeta3)

Answer 31 · 2022-02-28T22:20:28.000Z

comment:18

Replying to @slel:

Some more parts can be made symbolics-free,
though sometimes it makes things convoluted.

File src/sage/categories/algebra_functor.py

We can bypass using sqrt(5) to construct ZZ[sqrt(5)]:
    sage: G = GL(2, 7)
    sage: K = QuadraticField(5, 'srqt5')
    sage: Zr5 = K.order(K.gen())  # This is ZZ[sqrt(5)]

Or just ZZ[AA(5).sqrt()]

Answer 32 · 2022-02-28T22:23:57.000Z

Changed commit from e177b45 to bbe19ad

Answer 33 · 2022-02-28T22:23:57.000Z

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

`bbe19ad`	`src/sage/categories/algebra_functor.py: In doctest, use ZZ[AA(5).sqrt()] instead of ZZ[sqrt(5)] to avoid symbolics`

Answer 34 · 2022-02-28T22:27:12.000Z

Changed commit from bbe19ad to 5d82771

Answer 35 · 2022-02-28T22:27:12.000Z

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

`5d82771`	`src/sage/categories/coxeter_groups.py: In doctest, avoid using symbolics`

Answer 36 · 2022-02-28T22:27:39.000Z

comment:21

Replying to @slel:

File src/sage/categories/coxeter_groups.py

Using a polynomial variable can bypass symbolics here [...]

Thanks, I've made a similar change

Answer 37 · 2022-02-28T22:37:20.000Z

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

`3851908`	`src/sage/categories/pushout.py: In doctest, avoid using symbolics`

Answer 38 · 2022-02-28T22:37:20.000Z

Changed commit from 5d82771 to 3851908

Answer 39 · 2022-02-28T23:19:56.000Z

comment:23

I like ZZ[AA(5).sqrt()] but it does not name
the generator sqrt5 as ZZ[sqrt(5)]:

sage: ZZ[sqrt(5)]
Order in Number Field in sqrt5 with defining polynomial x^2 - 5 with sqrt5 = 2.236067977499790?
sage: ZZ[AA(5).sqrt()]
Order in Number Field in a with defining polynomial x^2 - 5 with a = 2.236067977499790?

so you need to change:

     sage: OG(OG.base_ring().basis()[1])
-    sqrt5*[1 0]
+    a*[1 0]
     [0 1]

Answer 40 · 2022-02-28T23:27:58.000Z

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

`595f99a`	`src/sage/categories/algebra_functor.py: Update doctest output`

Answer 41 · 2022-02-28T23:27:58.000Z

Changed commit from 3851908 to 595f99a

Answer 42 · 2022-03-01T06:27:58.000Z

Changed commit from 595f99a to ed59c28

Answer 43 · 2022-03-01T06:27:58.000Z

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

`ed59c28`	`src/sage/categories/pushout.py: Fix typo in doctest`

Answer 44 · 2022-03-01T06:30:30.000Z

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

`518bb96`	`src/sage/categories/modules_with_basis.py: Update doctest output`

Answer 45 · 2022-03-01T06:30:30.000Z

Changed commit from ed59c28 to 518bb96

Answer 46 · 2022-03-01T18:30:50.000Z

comment:27

The remaining doctest failure (and the pyflakes warnings) seen in the patchbot run are not from this ticket.

Answer 47 · 2022-03-02T22:50:41.000Z

Reviewer: Samuel Lelièvre

Answer 48 · 2022-03-08T17:46:30.000Z

Changed commit from 518bb96 to 4a1b489

Answer 49 · 2022-03-08T17:46:30.000Z

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

`4a1b489`	`Merge tag '9.6.beta4' into t/32609/remove_more_unnecessary_uses_of_sr_and_symbolic_functions_in_sage_combinat`

Answer 50 · 2022-03-09T11:26:00.000Z

comment:30

If bots go green or only report errors unrelated to this, positive review.

Answer 51 · 2022-03-09T22:15:14.000Z

comment:31

Patchbot failures and pyflakes warnings are unrelated.

Answer 52 · 2022-03-09T22:15:25.000Z

comment:32

Thank you!

Answer 53 · 2022-03-12T15:11:17.000Z

Changed branch from u/mkoeppe/remove_more_unnecessary_uses_of_sr_and_symbolic_functions_in_sage_combinat to 4a1b489