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

Allow completion() to return a lazy series for infinite precision #35345

Merged
merged 3 commits into from
Apr 6, 2023

Conversation

tscrim
Copy link
Collaborator

@tscrim tscrim commented Mar 25, 2023

📚 Description

We allow the completion() methods of polynomial rings to take infinite precision, in which case they return the corresponding lazy object. This also implements a completion() method to graded algebras and set a default equivalent to by degree for some general compatibility. (Full compatibility is not attempted due to general differences in the current implementation. This can be done on a later ticket.)

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2023

Codecov Report

Patch coverage: 85.71% and project coverage change: -0.01 ⚠️

Comparison is base (5330147) 88.61% compared to head (da45287) 88.60%.

❗ Current head da45287 differs from pull request most recent head 8c9a8a6. Consider uploading reports for the commit 8c9a8a6 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35345      +/-   ##
===========================================
- Coverage    88.61%   88.60%   -0.01%     
===========================================
  Files         2148     2148              
  Lines       398855   398863       +8     
===========================================
- Hits        353438   353431       -7     
- Misses       45417    45432      +15     
Impacted Files Coverage Δ
src/sage/rings/polynomial/polynomial_ring.py 94.17% <83.33%> (+0.02%) ⬆️
...c/sage/rings/polynomial/laurent_polynomial_ring.py 94.20% <85.71%> (+0.08%) ⬆️
src/sage/categories/graded_algebras_with_basis.py 97.22% <100.00%> (+0.07%) ⬆️

... and 20 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mantepse
Copy link
Collaborator

There seems to be a problem with the docbuild, could you check?

[sagemath_doc_html-none] [polynomia] docstring of sage.rings.polynomial.multi_polynomial_ring_base.MPolynomialRing_base.completion:10: WARNING: Bullet list ends without a blank line; unexpected unindent.
[sagemath_doc_html-none] [polynomia] docstring of sage.rings.polynomial.multi_polynomial_ring_base.MPolynomialRing_base.completion:11: WARNING: Block quote ends without a blank line; unexpected unindent.

Unrelated: allowing prec=oo everywhere is a different ticket, right? Here are some examples:

categories/number_fields.py\0125:        def zeta_function(self, prec=53,
categories/lie_algebras.py\0609:        def bch(self, X, Y, prec=None):
groups/matrix_gps/finitely_generated.py\0831:    def molien_series(self, chi=None, return_series=True, prec=20, variable='t'):
modular/modsym/space.py\0607:    def q_expansion_basis(self, prec=None, algorithm='default'):

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 25, 2023

I didn't check locally for the docbuild. I will fix that.

I can do those here too as it is a slight but very natural expansion of the scope.

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 25, 2023

The modular/modsym/space.py needs to do linear algebra over an infinite dimensional space. So it might be possible to make it work, it is a much more complicated and somewhat unrelated problem.

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 26, 2023

The zeta_function has to do with the precision of the underlying real number field. So it is irrelevant.

The bch requires doing computations in a set (generally non-graded) Lie algebra. It is possible that the computation could be modified for a graded Lie algebra and then done in the completion (in perhaps some special cases), but it is not really a computation that could be done lazily.

I changed the Molien series to handle infinite precision. This led to a natural extension of the conversion from the fraction field of the corresponding (Laurent) polynomial ring. For the Laurent series ring, this could be promoted to a coercion, but we cannot do that for a power series over a ring (although it should be fine over a field). I just did the easiest thing here that was natural and didn't bother with changing the coercions.

Something I noticed, there is a bit of an annoying thing about differentiating between sparse and dense polynomials. I am not sure of a good way to easily fix this, and that deserves its own issue/PR.

@mantepse
Copy link
Collaborator

Thank you for going through this!

At first I was puzzled that you don't try to convert x into a rational function immediately (instead of trying to convert it into a polynomial), but I guess it's easier the way you did it.

Thank you again!

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 26, 2023

Thank you.

We would run into problems if we tried to make a rational function first as we would then have to analyze the denominator. Plus the polynomial to fraction field conversion could be another source of subtleties, and I felt the polynomial conversion should be prioritized in the code.

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 29, 2023

Thank you!

@vbraun
Copy link
Member

vbraun commented Apr 1, 2023

Merge conflict

@tscrim tscrim force-pushed the rings/completion_method branch from 8c9a8a6 to 74f8c57 Compare April 3, 2023 13:20
@tscrim
Copy link
Collaborator Author

tscrim commented Apr 3, 2023

Trivial rebase (the Laurent polynomial ring implementation had moved files).

@vbraun vbraun merged commit 3884add into sagemath:develop Apr 6, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 7, 2023
@tscrim tscrim deleted the rings/completion_method branch April 28, 2023 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants