Add ABCs CommutativePolynomial, MPolynomial_libsingular, InfinitePolynomial; deprecate is_Polynomial, is_MPolynomial by mkoeppe · Pull Request #35076 · sagemath/sage

mkoeppe · 2023-02-11T18:49:47Z

📚 Description

Closes #32709

📝 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

…nstance testing

…mial for ABCs Polynomial, MPolynomial

…xpression

…th __classcall__

…e.rings.polynomial

… in isinstance tests

mkoeppe · 2023-02-11T18:50:34Z

PR fixes merge conflict in positively reviewed ticket #32709, setting to positive review.

codecov-commenter · 2023-02-11T21:18:44Z

Codecov Report

Base: 88.59% // Head: 88.58% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (ffb4647) compared to base (05329f6).
Patch coverage: 78.78% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35076      +/-   ##
===========================================
- Coverage    88.59%   88.58%   -0.02%     
===========================================
  Files         2140     2140              
  Lines       396998   397001       +3     
===========================================
- Hits        351740   351677      -63     
- Misses       45258    45324      +66

Impacted Files	Coverage Δ
...l/padics/polynomial_padic_capped_relative_dense.py	`67.97% <ø> (ø)`
.../sage/rings/polynomial/infinite_polynomial_ring.py	`87.46% <62.50%> (+0.13%)`	⬆️
...ge/rings/polynomial/infinite_polynomial_element.py	`83.15% <72.72%> (ø)`
src/sage/combinat/kazhdan_lusztig.py	`79.01% <100.00%> (ø)`
src/sage/combinat/schubert_polynomial.py	`92.30% <100.00%> (ø)`
src/sage/combinat/sf/sfa.py	`96.31% <100.00%> (ø)`
src/sage/crypto/mq/rijndael_gf.py	`80.14% <100.00%> (ø)`
src/sage/crypto/stream.py	`74.73% <100.00%> (ø)`
src/sage/groups/affine_gps/group_element.py	`94.69% <100.00%> (ø)`
.../differentiable/characteristic_cohomology_class.py	`81.40% <100.00%> (ø)`
... and 47 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…ial__mpolynomial_for_isinstance_testing SageMath version 9.8, Release Date: 2023-02-11

mkoeppe · 2023-02-12T05:43:45Z

Failures in sage/rings/polynomial/infinite_polynomial_element.py https://github.com/sagemath/sage/actions/runs/4153778889/jobs/7185627677#step:11:7560

…ePolynomial in isinstance tests, not InfinitePolynomial_sparse

…polynomial__mpolynomial_for_isinstance_testing SageMath version 10.0.beta0, Release Date: 2023-02-12 # Conflicts: # src/sage/combinat/schubert_polynomial.py

…ePolynomial) without _sparse

…polynomial__mpolynomial_for_isinstance_testing

tscrim

Let it be so, but I am still a bit worried about the continued increase in maintenance burden with the lack of specifications.

mkoeppe · 2023-02-17T01:46:06Z

A more positive take would be that the new class CommutativePolynomial now is a clear place where we can put abstract methods to specify an API common to single- and multivariate polynomials.

mkoeppe · 2023-02-17T01:46:29Z

Thanks for the review!

tscrim · 2023-02-17T01:55:02Z

A more positive take would be that the new class CommutativePolynomial now is a clear place where we can put abstract methods to specify an API common to single- and multivariate polynomials.

Indeed, this I am very happy about (a long time wish-list item from me). However, I am still not clear what makes the infinite polynomials deserving of an ABCDEFIT.

tscrim · 2023-02-17T01:55:57Z

Also, could you change the PR description to "Fixes #32709" instead of "Closes #32709" for the automatic closing of the issue?

mkoeppe · 2023-02-17T01:56:42Z

"Fixes", "Closes", and "Resolves" all have the same magic effect.

mkoeppe · 2023-02-17T01:57:22Z

https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue

tscrim · 2023-02-17T01:58:21Z

I see. Thanks.

mkoeppe · 2023-02-17T02:22:04Z

A more positive take would be that the new class CommutativePolynomial now is a clear place where we can put abstract methods to specify an API common to single- and multivariate polynomials.

Indeed, this I am very happy about (a long time wish-list item from me).

I have opened #35149 for keeping track of follow-ups in this direction.

tscrim · 2023-02-17T02:27:28Z

Thank you.

…polynomial__mpolynomial_for_isinstance_testing

…ut (trac->github)

…ucture_element__add_abcs_polynomial__mpolynomial_for_isinstance_testing

… is_Polynomial, is_MPolynomial

github-actions · 2023-03-03T18:58:57Z

Documentation preview for this PR is ready! 🎉
Built with commit: 3b65350

dimpase · 2023-06-27T12:49:50Z

src/sage/rings/polynomial/multi_polynomial.pyx


 def is_MPolynomial(x):
+    from sage.misc.superseded import deprecation
+    deprecation(32709, "the function is_MPolynomial is deprecated; use isinstance(x, sage.structure.element.MPolynomial) instead")


the deprecation message is wrong:
it should be sage.rings.polynomial.multi_polynomial.MPolynomial, no?

Matthias Koeppe added 15 commits January 22, 2023 15:39

sage.structure.element: Add ABCs Polynomial, MPolynomial, use for isi…

eb3a270

…nstance testing

Use ABC MPolynomial for isinstance testing

6696f5c

Fixups

79f7e62

sage.rings.polynomial: Make it a namespace package

ed050bb

sage.structure.element: Introduce common base class CommutativePolyno…

0613280

…mial for ABCs Polynomial, MPolynomial

sage.structure.element: Introduce ABC InfinitePolynomial

27e8889

src/sage/structure/element.pyx: Update hierarchy in documentation

57228f5

src/sage/structure/element.pyx: Add unique-direct-subclass tests

95e0adc

src/sage/structure/element.pyx: Add unique-direct-subclass test for E…

1fd625c

…xpression

InfinitePolynomial: Change from constructor function to base class wi…

960506b

…th __classcall__

Remove ABCs sage.structure.element.*Polynomial; introduce ABCs in sag…

fd54c82

…e.rings.polynomial

Replace use of implementation class from multi_polynomial_libsingular…

538c2a7

… in isinstance tests

src/sage/rings/polynomial/commutative_polynomial.pyx: Add doctests

0fcb6fb

InfinitePolynomial: Move _lmul_, _rmul_ here from subclasses

6e2adaf

InfinitePolynomial.__classcall_private__: Add doctest

cdab0df

Merge tag '9.8' into t/32709/sage_structure_element__add_abcs_polynom…

f7e9f41

…ial__mpolynomial_for_isinstance_testing SageMath version 9.8, Release Date: 2023-02-11

Matthias Koeppe added 5 commits February 12, 2023 14:01

src/sage/rings/polynomial/infinite_polynomial_element.py: Use Infinit…

e06e582

…ePolynomial in isinstance tests, not InfinitePolynomial_sparse

Merge tag '10.0.beta0' into t/32709/sage_structure_element__add_abcs_…

6b557c0

…polynomial__mpolynomial_for_isinstance_testing SageMath version 10.0.beta0, Release Date: 2023-02-12 # Conflicts: # src/sage/combinat/schubert_polynomial.py

src/sage/combinat/schubert_polynomial.py: Use isinstance(..., Infinit…

8f5b799

…ePolynomial) without _sparse

Merge branch 'develop' into t/32709/sage_structure_element__add_abcs_…

2d32a2e

…polynomial__mpolynomial_for_isinstance_testing

Merge branch 'develop' into t/32709/sage_structure_element__add_abcs_…

95b8793

…polynomial__mpolynomial_for_isinstance_testing

tscrim approved these changes Feb 17, 2023

View reviewed changes

Matthias Köppe added 4 commits February 19, 2023 14:28

Merge branch 'develop' into t/32709/sage_structure_element__add_abcs_…

488a8b1

…polynomial__mpolynomial_for_isinstance_testing

src/sage/rings/polynomial/polynomial_element.pyx: Update doctest outp…

ffb4647

…ut (trac->github)

Merge remote-tracking branch 'upstream/develop' into t/32709/sage_str…

2993660

…ucture_element__add_abcs_polynomial__mpolynomial_for_isinstance_testing

src/sage/quadratic_forms/quadratic_form.py: Replace use of deprecated…

3b65350

… is_Polynomial, is_MPolynomial

vbraun merged commit 2a7b4c9 into sagemath:develop Mar 13, 2023

dimpase reviewed Jun 27, 2023

View reviewed changes

dimpase mentioned this pull request Jun 27, 2023

incorrect deprecation messages for [M]Polynomial #35844

Closed

2 tasks

Uh oh!

Conversation

mkoeppe commented Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📚 Description

📝 Checklist

⌛ Dependencies

Uh oh!

mkoeppe commented Feb 11, 2023

Uh oh!

codecov-commenter commented Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mkoeppe commented Feb 12, 2023

Uh oh!

tscrim left a comment

Choose a reason for hiding this comment

Uh oh!

mkoeppe commented Feb 17, 2023

Uh oh!

mkoeppe commented Feb 17, 2023

Uh oh!

tscrim commented Feb 17, 2023

Uh oh!

tscrim commented Feb 17, 2023

Uh oh!

mkoeppe commented Feb 17, 2023

Uh oh!

mkoeppe commented Feb 17, 2023

Uh oh!

tscrim commented Feb 17, 2023

Uh oh!

mkoeppe commented Feb 17, 2023

Uh oh!

tscrim commented Feb 17, 2023

Uh oh!

github-actions bot commented Mar 3, 2023

Uh oh!

dimpase Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mkoeppe commented Feb 11, 2023 •

edited

Loading

codecov-commenter commented Feb 11, 2023 •

edited

Loading