Laurent polynomial/series modularization fixes #35554

mkoeppe · 2023-04-23T21:20:32Z

📚 Description

We remove several obstacles to modularization:

sage.data_structures.stream importing from sage.combinat.sf
eager module-level imports in sage.rings.bigoh, importing LaurentSeries, PuiseuxSeries, padics.
LaurentSeriesRing._element_constructor_ unconditionally importing sage.libs.pari.all just for an isinstance test
import of factorial from sage.functions (only the version from sage.arith is needed)
multivariate implementation of Laurent polynomials (with compile-time dependency on sage.matrix) mixed with the univariate implementation

Also adding # optional tags for the doctests that depend on sage.rings.finite_rings, sage.symbolic.

Part of:

Meta-ticket: Modularize sagelib into separate distributions (pip-installable packages) sagemath-... for Sage 10.x #29705

📝 Checklist

The title is concise, informative, and self-explanatory.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

…t_polynomial

…ixes

…metics

…pdate doctest output

mantepse · 2023-04-24T05:56:37Z

I think it would make much more sense to move symmetric functions to sage.algebras. It seems to me like a historic accident that they reside in sage.combinat.

mkoeppe · 2023-04-24T19:22:19Z

I think it would make much more sense to move symmetric functions to sage.algebras. It seems to me like a historic accident that they reside in sage.combinat.

That would be a plausible redesign. However, I'm using these feature tags in a looser way: Most of sage.algebras and sage.combinat will likely be shipped in the same distribution package, so use of "algebras whose bases are of combinatorial nature" could still be marked # optional - sage.combinat.

…zation_fixes

kwankyu · 2023-06-02T05:06:30Z

You use two idioms

lazy_import('sage.rings.padics.padic_generic_element', 'pAdicGenericElement')

and

try:
    from .laurent_series_ring_element import LaurentSeries
except ImportError:
    LaurentSeries = ()

to modify module level imports of classes, for modularization. How are they different in terms of usage?

kwankyu · 2023-06-02T05:16:22Z

src/sage/rings/lazy_series.py

-    sage: check(L, gen(), valuation=-5)
+    sage: L.<z> = LazyLaurentSeriesRing(GF(2))                                          # optional - sage.rings.finite_rings
+    sage: check(L, lambda n: n, valuation=-5)                                           # optional - sage.rings.finite_rings
+    sage: check(L, gen(), valuation=-5)                                                 # optional - sage.rings.finite_rings

    sage: L = LazyDirichletSeriesRing(QQbar, "s")
    sage: check(L, lambda n: n, valuation=2)
    sage: check(L, gen(), valuation=2)



sage.rings.number_field?

By the way, this being singular is not consistent with sage.rings.finite_rings being plural...

Yes, I'm also mildly annoyed by this inconsistency

Done in b9bf5bb

kwankyu · 2023-06-02T05:30:04Z

src/sage/rings/polynomial/laurent_polynomial.pyx

-            sage: L.<x,y> = LaurentPolynomialRing(QQ) # indirect doctest
-            sage: x*y
+            sage: L.<x,y> = LaurentPolynomialRing(QQ)  # indirect doctest               # optional - sage.modules
+            sage: x*y                                                                   # optional - sage.modules
            x*y


Does this (just constructing the ring and an element) depend on sage.modules? Then the distribution package that installs laurent_polynomial.pyx should install sage.modules. No?

kwankyu · 2023-06-02T05:33:38Z

src/sage/rings/polynomial/laurent_polynomial.pyx

            False
-            sage: (2 + 4*u + 2*u^2).is_square()
+            sage: (2 + 4*u + 2*u^2).is_square()                                         # optional - sage.rings.number_field
            True


Hmm. So perhaps sage.modules is a dependency of only multi-variate Laurent polynomial ring.

yes, see below

kwankyu · 2023-06-02T05:35:21Z

src/sage/rings/polynomial/laurent_polynomial_mpair.pyx

+
+            sage: F.<t> = GF(4)                                                         # optional - sage.rings.finite_rings
+            sage: LF.<a,b> = LaurentPolynomialRing(F)                                   # optional - sage.rings.finite_rings
+            sage: rho = LF.hom([b,a], base_map=F.frobenius_endomorphism())              # optional - sage.rings.finite_rings


No sage.modules here? I am getting confused...

This whole file is only shipped with sagemath-modules (#32432) because it cimports from sage.matrix.
(I split out this file from laurent_polynomial.pyx for this reason.)

Thus this whole business of adding (and not-adding) optional tags requires understanding of the hierarchy of distribution packages and which files are included in which distribution packages. This is very unlike with other doctest tags, which the author of the doctests can decide solely from his/her understanding of the code.

If the business is one time work (that is done and forgot), no matter. But every author that makes changes to doctests should be aware of the modularization structure of the sage codebase, not to break distribution packages.

I think we should later provide some tools or documentation to make the business easy and obvious.

Would Github actions testing distribution packages provide information to help authors decide on optional tags?

Yes, there is a clear gap in the documentation - both in the code and in the formatted documentation.

In the code, the plan is to annotate each file with the name of the distribution that it is shipped by: # sage_setup: distribution = sagemath-combinat etc.; #35663 adds many of such annotations; this will enable consistency checks.

In the formatted documentation, file-level annotations # sage_setup: distribution and # sage.doctest: optional are not presented at all at the moment; in contrast to the line-level # optional annotations.
I don't have a clear plan yet how to improve it.

Yes, GH Actions will run the tests for the distributions after #35095, and this will flag any mistakes.

OK. Sounds nice.

mkoeppe · 2023-06-02T18:01:04Z

You use two idioms
lazy_import('sage.rings.padics.padic_generic_element', 'pAdicGenericElement')
and
try:
    from .laurent_series_ring_element import LaurentSeries
except ImportError:
    LaurentSeries = ()  
to modify module level imports of classes, for modularization. How are they different in terms of usage?

They have different performance characteristics, as explained in https://sagemath-tobias.netlify.app/developer/packaging_sage_library.html#module-level-runtime-dependencies

I think I tend to use the second idiom when the code that uses it seemed to be concerned about performance; but I am not sure that I have been entirely consistent with it.

The first idiom, on the other hand, looks more elegant; and by adding a feature=... argument to lazy_import, we can in the future provide users with installation information about the dependencies. (eager_import, proposed in #33820, could bring the second idiom on par with this.)

github-actions · 2023-06-02T19:29:36Z

Documentation preview for this PR (built with commit b9bf5bb) is ready! 🎉

kwankyu · 2023-06-02T21:34:04Z

...eager_import, proposed in #33820, could bring the second idiom on par with this.

Good idea. Or perhaps you may add eager flag to lazy_import instead of introducing a new import statement.

kwankyu

LGTM.

mkoeppe · 2023-06-02T22:22:34Z

Thanks for the review!

Matthias Koeppe added 3 commits April 23, 2023 14:15

sage.rings: Modularization fixes for Laurent polynomials, series

f10c1ef

sage.rings.polynomial.laurent_polynomial_mpair: Split out from lauren…

21b23bf

…t_polynomial

sage.data_structures.stream, sage.rings.lazy_series: Modularization f…

f094cdc

…ixes

mkoeppe added c: algebra t: refactoring labels Apr 23, 2023

mkoeppe self-assigned this Apr 23, 2023

Fix imports

eaef6e6

mkoeppe force-pushed the laurent_modularization_fixes branch from 48d4ecf to eaef6e6 Compare April 23, 2023 23:12

mkoeppe added the s: needs review label Apr 23, 2023

mkoeppe requested a review from mantepse April 23, 2023 23:12

Matthias Koeppe added 3 commits April 23, 2023 16:20

src/sage/rings/polynomial/laurent_polynomial_mpair.pyx: Docstring cos…

cdb4755

…metics

More # optional

e8df730

src/sage/combinat/root_system/root_lattice_realization_algebras.py: U…

986e3b8

…pdate doctest output

Merge remote-tracking branch 'upstream/develop' into laurent_modulari…

38468d6

…zation_fixes

kwankyu reviewed Jun 2, 2023

View reviewed changes

src/sage/rings/lazy_series.py: Add # optional - sage.rings.number_field

b9bf5bb

kwankyu removed the s: needs review label Jun 2, 2023

kwankyu approved these changes Jun 2, 2023

View reviewed changes

kwankyu added the s: positive review label Jun 2, 2023

vbraun merged commit 22999ff into sagemath:develop Jun 3, 2023

mkoeppe added this to the sage-10.1 milestone Jun 3, 2023

mkoeppe removed the s: positive review label Jun 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Laurent polynomial/series modularization fixes #35554

Laurent polynomial/series modularization fixes #35554

mkoeppe commented Apr 23, 2023 •

edited

Loading

mantepse commented Apr 24, 2023

mkoeppe commented Apr 24, 2023

kwankyu commented Jun 2, 2023

kwankyu Jun 2, 2023 •

edited

Loading

mkoeppe Jun 2, 2023

mkoeppe Jun 2, 2023

kwankyu Jun 2, 2023

mkoeppe Jun 2, 2023

kwankyu Jun 2, 2023

mkoeppe Jun 2, 2023

kwankyu Jun 2, 2023

mkoeppe Jun 2, 2023

kwankyu Jun 2, 2023 •

edited

Loading

mkoeppe Jun 2, 2023

kwankyu Jun 2, 2023

mkoeppe commented Jun 2, 2023

github-actions bot commented Jun 2, 2023

kwankyu commented Jun 2, 2023

kwankyu left a comment

mkoeppe commented Jun 2, 2023

Laurent polynomial/series modularization fixes #35554

Laurent polynomial/series modularization fixes #35554

Conversation

mkoeppe commented Apr 23, 2023 • edited Loading

📚 Description

📝 Checklist

⌛ Dependencies

mantepse commented Apr 24, 2023

mkoeppe commented Apr 24, 2023

kwankyu commented Jun 2, 2023

kwankyu Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwankyu Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkoeppe commented Jun 2, 2023

github-actions bot commented Jun 2, 2023

kwankyu commented Jun 2, 2023

kwankyu left a comment

Choose a reason for hiding this comment

mkoeppe commented Jun 2, 2023

mkoeppe commented Apr 23, 2023 •

edited

Loading

kwankyu Jun 2, 2023 •

edited

Loading

kwankyu Jun 2, 2023 •

edited

Loading