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.

Sign up for GitHub

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

Jump to bottom

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

Closed
mkoeppe opened this issue Oct 17, 2021 · 103 comments · Fixed by #35076
Closed

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

mkoeppe opened this issue Oct 17, 2021 · 103 comments · Fixed by #35076
Labels
p: major / 3 t: enhancement t: refactoring
Milestone
sage-10.0

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 17, 2021 •
edited
Loading

isinstance(x, [M]Polynomial) replaces the use of is_Polynomial, is_MPolynomial (deprecated here).

The new class CommutativePolynomial is an ABC that is a common base of Polynomial, MPolynomial, and InfinitePolynomial. We introduce it here although there is currently no use for isinstance(x, CommutativePolynomial) in the library.

The new class InfinitePolynomial is a common base class of InfinitePolynomial_sparse and InfinitePolynomial_dense. Methods shared between both classes are moved to the new class.

The new class MPolynomial_libsingular is an ABCDEFIT (ABC defined exclusively for isinstance tests) with a unique direct subclass (see Meta-ticket #32414 and https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#module-level-runtime-dependencies for this design pattern). Its purpose is to replace uses of isinstance(x, MPolynomial_libsingular) throughout the library. This eliminates hard run-time dependencies on Singular.

We also make sage.rings.polynomial a namespace package; this is needed because element implementations depend on various libraries.

CC: @dimpase @mezzarobba @videlec @tscrim

Component: refactoring

Author: Matthias Koeppe

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/32709
@mkoeppe mkoeppe added this to the sage-9.5 milestone Oct 17, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 14, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Mar 5, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2022

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2022

Branch: u/mkoeppe/sage_structure_element__add_abcs_polynomial__mpolynomial_for_isinstance_testing

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 6, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

4185472Fixups

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 6, 2022

Commit: 4185472

@mkoeppe

This comment has been minimized.

Sign in to view
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 6, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

46d8351sage.rings.polynomial: Make it a namespace package

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 6, 2022

Changed commit from 4185472 to 46d8351

@mkoeppe

This comment has been minimized.

Sign in to view
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

29d91adsage.structure.element: Add ABCs Polynomial, MPolynomial, use for isinstance testing
ed9e0e4Use ABC MPolynomial for isinstance testing
f4b0aa7Fixups
e21267fsage.rings.polynomial: Make it a namespace package

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2022

Changed commit from 46d8351 to e21267f

@tscrim
Copy link
Collaborator

tscrim commented Dec 23, 2022

comment:14

Merge failure.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 23, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

e156cadMerge tag '9.8.beta6' into t/32709/sage_structure_element__add_abcs_polynomial__mpolynomial_for_isinstance_testing

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 23, 2022

Changed commit from e21267f to e156cad

@tscrim
Copy link
Collaborator

tscrim commented Jan 6, 2023

comment:17

Thank you for doing this. This is something that I have been thinking about having for a while. The only other thing I would like would be a common ABC for both univariate and multivariate polynomials.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 6, 2023

comment:18

Currently we have:

cdef class Polynomial(CommutativeAlgebraElement):

but

cdef class MPolynomial(CommutativeRingElement):

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 22, 2023

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

19255e8WIP - Change design

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 22, 2023

comment:85

I'm moving the modularization boundary into sage.rings.polynomial. The additions to sage.structure.element are gone.

The new module sage.rings.polynomial.commutative_polynomial provides a new, currently trivial, ABC.

@mkoeppe

This comment has been minimized.

Sign in to view
@mkoeppe mkoeppe changed the title sage.structure.element: Add ABCs CommutativePolynomial, Polynomial, MPolynomial, InfinitePolynomial for isinstance testing Add ABCs CommutativePolynomial, MPolynomial_libsingular; deprecate is_Polynomial, is_MPolynomial Jan 22, 2023
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2023

Changed commit from 19255e8 to 0b6d19e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2023

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0b6d19eRemove ABCs sage.structure.element.*Polynomial; introduce ABCs in sage.rings.polynomial

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2023

Changed commit from 0b6d19e to fd54c82

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2023

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fd54c82Remove ABCs sage.structure.element.*Polynomial; introduce ABCs in sage.rings.polynomial

@mkoeppe

This comment has been minimized.

Sign in to view
@mkoeppe mkoeppe changed the title Add ABCs CommutativePolynomial, MPolynomial_libsingular; deprecate is_Polynomial, is_MPolynomial Add ABCs CommutativePolynomial, MPolynomial_libsingular, InfinitePolynomial; deprecate is_Polynomial, is_MPolynomial Jan 23, 2023
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2023

Branch pushed to git repo; I updated commit sha1. New commits:

538c2a7Replace use of implementation class from multi_polynomial_libsingular in isinstance tests
0fcb6fbsrc/sage/rings/polynomial/commutative_polynomial.pyx: Add doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2023

Changed commit from fd54c82 to 0fcb6fb

@tscrim
Copy link
Collaborator

tscrim commented Jan 25, 2023

comment:92

Thank you. Last two things, both for the infinite polynomials:

  1. __classcall_private__ needs doctests.
  2. The _rmul_ and _lmul_ can be refactored out to use type(self).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 25, 2023

Branch pushed to git repo; I updated commit sha1. New commits:

6e2adafInfinitePolynomial: Move `_lmul_`, `_rmul_` here from subclasses
cdab0dfInfinitePolynomial.__classcall_private__: Add doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 25, 2023

Changed commit from 0fcb6fb to cdab0df

@tscrim
Copy link
Collaborator

tscrim commented Jan 25, 2023

comment:94

Thank you. LGTM.

@tscrim
Copy link
Collaborator

tscrim commented Jan 25, 2023

Reviewer: Travis Scrimshaw

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 25, 2023

comment:95

Thanks!

@mkoeppe mkoeppe modified the milestones: sage-9.8, sage-9.9 Jan 29, 2023
@vbraun
Copy link
Member

vbraun commented Feb 11, 2023

Merge conflict, please create a corresponding github PR to fix.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 11, 2023

Removed branch from description; superseded by PR #35076.

mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 12, 2023
@mkoeppe
Merge sagemath#32709
daf2e53
vbraun pushed a commit that referenced this issue Mar 13, 2023
gh-35076: Add ABCs CommutativePolynomial, MPolynomial_libsingular, In…
2a7b4c9 
…finitePolynomial; deprecate is_Polynomial, is_MPolynomial

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes #1234" use "Introduce new method to
calculate 1+1"
-->
### 📚 Description

<!-- Describe your changes here in detail -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If it resolves an open issue, please link to the issue here. For
example "Closes #1337" -->
Closes #32709

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

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

### ⌛ Dependencies
<!-- List all open pull requests that this PR logically depends on -->
<!--
- #xyz: short description why this is a dependency
- #abc: ...
-->
    
URL: #35076
Reported by: Matthias Köppe
Reviewer(s): Travis Scrimshaw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
p: major / 3 t: enhancement t: refactoring
Projects
None yet
Milestone
sage-10.0
3 participants
@vbraun @tscrim @mkoeppe