Skip to content

Comments

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

Merged
vbraun merged 9 commits intosagemath:developfrom
mkoeppe:refactor_rings
Apr 1, 2023
Merged

sage.rings.polynomial.laurent_polynomial_ring_base: Split out from .laurent_polynomial_ring#35229
vbraun merged 9 commits intosagemath:developfrom
mkoeppe:refactor_rings

Conversation

@mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Mar 5, 2023 •
edited
Loading

📚 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 •
edited
Loading

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.

tscrim
tscrim requested changes Mar 9, 2023
View reviewed changes
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__.

src/sage/rings/polynomial/laurent_polynomial_ring_base.py Outdated
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

@tscrim tscrim Mar 9, 2023

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
Contributor Author

@mkoeppe mkoeppe Mar 9, 2023

Choose a reason for hiding this comment

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

Not my text

tscrim
tscrim approved these changes Mar 9, 2023
View reviewed changes
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
Contributor Author

mkoeppe commented Mar 9, 2023

Thanks!

@mkoeppe
Merge tag '10.0.beta4' into refactor_rings
d99a785 
SageMath version 10.0.beta4, Release Date: 2023-03-12
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 13, 2023

Thanks again!

@github-actions
Copy link

github-actions bot commented Mar 13, 2023

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
Contributor Author

mkoeppe commented Mar 28, 2023

This one is also unrelated.

@mkoeppe
Merge tag '10.0.beta6' into refactor_rings
9edf9dd 
SageMath version 10.0.beta6, Release Date: 2023-03-26
@vbraun vbraun merged commit 3b8f20e into sagemath:develop Apr 1, 2023
@fchapoton fchapoton mentioned this pull request Oct 17, 2025
Merged
2 tasks
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 18, 2025
4d160f6 
sagemathgh-41058: remove some deprecated stuff in rings/
    
after

File: src/sage/rings/polynomial/laurent_polynomial_ring.py ; PR: sagemath#35229
closed in 2023
File: src/sage/rings/polynomial/groebner_fan.py ; PR: sagemath#33468 closed in
2022
File: src/sage/rings/polynomial/multi_polynomial.pyx ; PR: sagemath#32709 closed
in 2023
File: src/sage/rings/number_field/number_field.py ; PR: sagemath#33386 closed in
2022

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
    
URL: sagemath#41058
Reported by: Frédéric Chapoton
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 19, 2025
cd68979 
sagemathgh-41058: remove some deprecated stuff in rings/
    
after

File: src/sage/rings/polynomial/laurent_polynomial_ring.py ; PR: sagemath#35229
closed in 2023
File: src/sage/rings/polynomial/groebner_fan.py ; PR: sagemath#33468 closed in
2022
File: src/sage/rings/polynomial/multi_polynomial.pyx ; PR: sagemath#32709 closed
in 2023
File: src/sage/rings/number_field/number_field.py ; PR: sagemath#33386 closed in
2022

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
    
URL: sagemath#41058
Reported by: Frédéric Chapoton
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 20, 2025
d062b04 
sagemathgh-41058: remove some deprecated stuff in rings/
    
after

File: src/sage/rings/polynomial/laurent_polynomial_ring.py ; PR: sagemath#35229
closed in 2023
File: src/sage/rings/polynomial/groebner_fan.py ; PR: sagemath#33468 closed in
2022
File: src/sage/rings/polynomial/multi_polynomial.pyx ; PR: sagemath#32709 closed
in 2023
File: src/sage/rings/number_field/number_field.py ; PR: sagemath#33386 closed in
2022

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
    
URL: sagemath#41058
Reported by: Frédéric Chapoton
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 26, 2025
4d154a9 
sagemathgh-41058: remove some deprecated stuff in rings/
    
after

File: src/sage/rings/polynomial/laurent_polynomial_ring.py ; PR: sagemath#35229
closed in 2023
File: src/sage/rings/polynomial/groebner_fan.py ; PR: sagemath#33468 closed in
2022
File: src/sage/rings/polynomial/multi_polynomial.pyx ; PR: sagemath#32709 closed
in 2023
File: src/sage/rings/number_field/number_field.py ; PR: sagemath#33386 closed in
2022

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
    
URL: sagemath#41058
Reported by: Frédéric Chapoton
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 26, 2025
26c140b 
sagemathgh-41058: remove some deprecated stuff in rings/
    
after

File: src/sage/rings/polynomial/laurent_polynomial_ring.py ; PR: sagemath#35229
closed in 2023
File: src/sage/rings/polynomial/groebner_fan.py ; PR: sagemath#33468 closed in
2022
File: src/sage/rings/polynomial/multi_polynomial.pyx ; PR: sagemath#32709 closed
in 2023
File: src/sage/rings/number_field/number_field.py ; PR: sagemath#33386 closed in
2022

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
    
URL: sagemath#41058
Reported by: Frédéric Chapoton
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tscrim tscrim tscrim approved these changes

@mezzarobba mezzarobba Awaiting requested review from mezzarobba

Assignees

@mkoeppe mkoeppe

Labels

c: commutative algebra t: refactoring

Projects

None yet

Milestone

sage-10.0

Development

Successfully merging this pull request may close these issues.

4 participants

@mkoeppe @codecov-commenter @vbraun @tscrim