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

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

Closed
mkoeppe opened this issue Oct 2, 2021 · 53 comments
Closed

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 2, 2021

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

@mkoeppe mkoeppe added this to the sage-9.5 milestone Oct 2, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 27, 2022

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 27, 2022

Commit: 04855a1

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 27, 2022

New commits:

6bd6903PowerSeriesRing_generic._element_constructor_: Only import sage.symbolic.expression.SymbolicSeries after isinstance test with ABC Expression
9730b10src/sage/categories/metric_spaces.py: Mark doctests using HyperbolicPlane # optional - sage.symbolic
dca7113In doctests, replace floor(.../...) by //
44ac5absrc/sage/categories/vector_bundles.py: Mark doctests # optional - sage.symbolic
71303ddsrc/sage/matrix/operation_table.py: Use math.log, log10 insteadd of importing from sage.misc.functional
04855a1src/sage/groups/matrix_gps/coxeter_group.py: Do not use sage.symbolic for easy cases of cos(pi/x)

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Remove more unnecessary uses of SR and symbolic functions in sage.combinat Remove more unnecessary uses of SR and symbolic functions in sage.combinat, sage.categories etc. Feb 27, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 27, 2022

Author: Matthias Koeppe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 27, 2022

Changed commit from 04855a1 to ecad015

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 27, 2022

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

Changed commit from ecad015 to 2511e3e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

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

5d2cc6bPowerSeriesRing_generic._element_constructor_: Only import sage.symbolic.expression.SymbolicSeries after isinstance test with ABC Expression
dc8cc8csrc/sage/categories/metric_spaces.py: Mark doctests using HyperbolicPlane # optional - sage.symbolic
9ca27e7In doctests, replace floor(.../...) by //
444b659src/sage/categories/vector_bundles.py: Mark doctests # optional - sage.symbolic
63b0ad8src/sage/matrix/operation_table.py: Use math.log, log10 insteadd of importing from sage.misc.functional
13c5092src/sage/groups/matrix_gps/coxeter_group.py: Do not use sage.symbolic for easy cases of cos(pi/x)
2511e3esrc/sage/groups/matrix_gps/coxeter_group.py: Do not use sage.symbolic for easy cases of cos(pi/x) (fixup)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

Changed commit from 2511e3e to ea8d379

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

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

e3b36fasrc/sage/categories/algebra_functor.py: Make doctests using sqrt # optional - sage.symbolic
ea8d379src/sage/categories/rings.py: Mark doctests using sqrt # optional - sage.symbolic

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

Changed commit from ea8d379 to 9cae6cb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

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

994f47bsrc/sage/categories/number_fields.py: Remove abuse of predefined x in doctests
4a1fc16src/sage/structure/element.pyx: Remove abuse of predefined x in doctests
c6714aasrc/sage/categories/coxeter_groups.py: Mark a doctest # optional - sage.symbolic
30fa675src/sage/categories/pushout.py: Remove abuse of predefined x in doctests
3cd7195src/sage/categories/modules_with_basis.py: In doctest, import factorial from sage.arith.misc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

Changed commit from 9cae6cb to 3cd7195

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

Changed commit from 3cd7195 to 2445b36

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

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

fe0c388src/sage/categories/lie_algebras.py: Mark doctests using manifolds # optional - sage.symbolic

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

Changed commit from 2445b36 to fe0c388

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

Changed commit from fe0c388 to e197558

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

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

e197558src/sage/categories/finite_dimensional_nilpotent_lie_algebras_with_basis.py: Mark doctests using manifolds # optional - sage.symbolic

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

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

3ede66asrc/sage/categories/finite_permutation_groups.py: Mark a doctest # optional - sage.symbolic
caf233esrc/sage/categories/metric_spaces.py: Mark a doctest # optional - sage.symbolic

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 28, 2022

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()]

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

Changed commit from e177b45 to bbe19ad

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

Changed commit from bbe19ad to 5d82771

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

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

5d82771src/sage/categories/coxeter_groups.py: In doctest, avoid using symbolics

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 28, 2022

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

Changed commit from 5d82771 to 3851908

@slel
Copy link
Member

slel commented Feb 28, 2022

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]

@slel slel changed the title Remove more unnecessary uses of SR and symbolic functions in sage.combinat, sage.categories etc. Remove or tag uses of SR and symbolic functions in combinat, categories, etc. Feb 28, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

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

595f99asrc/sage/categories/algebra_functor.py: Update doctest output

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2022

Changed commit from 3851908 to 595f99a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2022

Changed commit from 595f99a to ed59c28

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2022

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

ed59c28src/sage/categories/pushout.py: Fix typo in doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2022

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

518bb96src/sage/categories/modules_with_basis.py: Update doctest output

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2022

Changed commit from ed59c28 to 518bb96

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 1, 2022

comment:27

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 2, 2022

Reviewer: Samuel Lelièvre

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 8, 2022

Changed commit from 518bb96 to 4a1b489

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 8, 2022

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

4a1b489Merge tag '9.6.beta4' into t/32609/remove_more_unnecessary_uses_of_sr_and_symbolic_functions_in_sage_combinat

@slel
Copy link
Member

slel commented Mar 9, 2022

comment:30

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 9, 2022

comment:31

Patchbot failures and pyflakes warnings are unrelated.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 9, 2022

comment:32

Thank you!

@vbraun
Copy link
Member

vbraun commented Mar 12, 2022

@vbraun vbraun closed this as completed in 045292b Mar 12, 2022
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 17, 2023
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants