Add ABCs CommutativePolynomial, MPolynomial_libsingular, InfinitePolynomial; deprecate is_Polynomial, is_MPolynomial #35076

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

@@ -20,6 +20,9 @@ from itertools import chain, islice
 from sage.misc.misc_c import prod

 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

mkoeppe added p: major / 3 s: positive review t: refactoring labels Feb 11, 2023

mkoeppe added s: needs work and removed s: positive review labels Feb 12, 2023

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

mkoeppe force-pushed the t/32709/sage_structure_element__add_abcs_polynomial__mpolynomial_for_isinstance_testing branch from d4a46aa to f7e9f41 Compare February 12, 2023 00:12

mkoeppe removed the s: needs work label Feb 12, 2023

mkoeppe added the s: needs work label Feb 12, 2023

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

e06e582

…ePolynomial in isinstance tests, not InfinitePolynomial_sparse

mkoeppe added s: needs review and removed s: needs work labels Feb 12, 2023

mkoeppe self-assigned this Feb 12, 2023

mkoeppe requested a review from tscrim February 12, 2023 22:03

Matthias Koeppe added 3 commits February 12, 2023 16:23

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

mkoeppe added this to the sage-10.0 milestone Feb 13, 2023

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

mkoeppe added s: positive review and removed s: needs review labels Feb 17, 2023

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

mkoeppe removed the s: positive review label 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ABCs CommutativePolynomial, MPolynomial_libsingular, InfinitePolynomial; deprecate is_Polynomial, is_MPolynomial #35076

Add ABCs CommutativePolynomial, MPolynomial_libsingular, InfinitePolynomial; deprecate is_Polynomial, is_MPolynomial #35076

mkoeppe commented Feb 11, 2023 •

edited

Loading

mkoeppe commented Feb 11, 2023

codecov-commenter commented Feb 11, 2023 •

edited

Loading

mkoeppe commented Feb 12, 2023

tscrim left a comment

mkoeppe commented Feb 17, 2023

mkoeppe commented Feb 17, 2023

tscrim commented Feb 17, 2023

tscrim commented Feb 17, 2023

mkoeppe commented Feb 17, 2023

mkoeppe commented Feb 17, 2023

tscrim commented Feb 17, 2023

mkoeppe commented Feb 17, 2023

tscrim commented Feb 17, 2023

github-actions bot commented Mar 3, 2023

dimpase Jun 27, 2023

Add ABCs CommutativePolynomial, MPolynomial_libsingular, InfinitePolynomial; deprecate is_Polynomial, is_MPolynomial #35076

Add ABCs CommutativePolynomial, MPolynomial_libsingular, InfinitePolynomial; deprecate is_Polynomial, is_MPolynomial #35076

Conversation

mkoeppe commented Feb 11, 2023 • edited Loading

📚 Description

📝 Checklist

⌛ Dependencies

mkoeppe commented Feb 11, 2023

codecov-commenter commented Feb 11, 2023 • edited Loading

Codecov Report

mkoeppe commented Feb 12, 2023

tscrim left a comment

Choose a reason for hiding this comment

mkoeppe commented Feb 17, 2023

mkoeppe commented Feb 17, 2023

tscrim commented Feb 17, 2023

tscrim commented Feb 17, 2023

mkoeppe commented Feb 17, 2023

mkoeppe commented Feb 17, 2023

tscrim commented Feb 17, 2023

mkoeppe commented Feb 17, 2023

tscrim commented Feb 17, 2023

github-actions bot commented Mar 3, 2023

dimpase Jun 27, 2023

Choose a reason for hiding this comment

mkoeppe commented Feb 11, 2023 •

edited

Loading

codecov-commenter commented Feb 11, 2023 •

edited

Loading