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

sage.rings.polynomial.laurent_polynomial_ring_base: Split out from .laurent_polynomial_ring #35229

Merged
merged 9 commits into from
Apr 1, 2023

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Mar 5, 2023

πŸ“š Description

We move the abstract base class LaurentPolynomialRing_generic to a separate module.
This is for modularization purposes (meta-ticket #32414).

We also deprecate the function is_LaurentPolynomialRing and replace the (few) uses by isinstance.

πŸ“ 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.

βŒ› Dependencies

@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2023

Codecov Report

Patch coverage: 89.68% and project coverage change: +0.01 πŸŽ‰

Comparison is base (f449b14) 88.60% compared to head (d99a785) 88.61%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35229      +/-   ##
===========================================
+ Coverage    88.60%   88.61%   +0.01%     
===========================================
  Files         2148     2149       +1     
  Lines       398653   398661       +8     
===========================================
+ Hits        353241   353288      +47     
+ Misses       45412    45373      -39     
Impacted Files Coverage Ξ”
...e/rings/polynomial/laurent_polynomial_ring_base.py 88.59% <88.59%> (ΓΈ)
src/sage/categories/pushout.py 89.50% <100.00%> (+0.01%) ⬆️
src/sage/rings/fraction_field.py 96.94% <100.00%> (ΓΈ)
src/sage/rings/laurent_series_ring.py 95.71% <100.00%> (ΓΈ)
src/sage/rings/localization.py 96.11% <100.00%> (ΓΈ)
...c/sage/rings/polynomial/laurent_polynomial_ring.py 92.72% <100.00%> (-1.40%) ⬇️

... and 26 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.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, the rest is just moving stuff line-for-line, which is okay. I know this is for modularization purposes, but it looks weird that the main global entry point is separate from the generic class. I would consider changing the generic class name to LaurentPolynomialRing and moving the dispatch function into its __classcall_private__.

Comment on lines 7 to 11
We implement it as a quotient ring

.. MATH::

R[x_1, y_1, x_2, y_2, \ldots, x_n, y_n] / (x_1 y_1 - 1, x_2 y_2 - 1, \ldots, x_n y_n - 1).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not true: It is not in the category of quotients, which reflects that it doesn't have methods like ambient() and lift(), nor does it reflect the internal structure. We have implemented it as a localization. Although it is fine to say it is isomorphic to this quotient ring; not that anything needs to be said here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not my text

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. LGTM.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 9, 2023

Thanks!

SageMath version 10.0.beta4, Release Date: 2023-03-12
@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 13, 2023

Thanks again!

@github-actions
Copy link

Documentation preview for this PR is ready! πŸŽ‰
Built with commit: d99a785

@vbraun
Copy link
Member

vbraun commented Mar 28, 2023

**********************************************************************
File "src/sage/interfaces/r.py", line 697, in sage.interfaces.r.R.__reduce__
Failed example:
    rlr, t = r.__reduce__()  # optional - rpy2
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/src/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/src/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.interfaces.r.R.__reduce__[0]>", line 1, in <module>
        rlr, t = r.__reduce__()  # optional - rpy2
                 ^^^^^^^^^^^^^^
      File "/home/release/Sage/local/var/lib/sage/venv-python3.11.1/lib/python3.11/copyreg.py", line 76, in _reduce_ex
        raise TypeError(f"cannot pickle {cls.__name__!r} object")
    TypeError: cannot pickle 'LazyImport' object
**********************************************************************

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 28, 2023

This one is also unrelated.

SageMath version 10.0.beta6, Release Date: 2023-03-26
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.

None yet

4 participants