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

Ensure degree and total degree return Integer type for MPolynomial_polydict class #37611

Merged

Conversation

GiacomoPope
Copy link
Contributor

@GiacomoPope GiacomoPope commented Mar 14, 2024

This is a follow up PR to #37605 which ensures that degree() and total_degree() return a sage Integer type instead of a python int for the MPolynomial_polydict class.

However, f.degrees() returns something of type sage.rings.polynomial.polydict.ETuple which has elements as int. I am loathed to change this to return Integer in case of performance regression, so the following status of MPolynomial_polydict is the following:

sage: R.<x, y> = PolynomialRing(QQbar)
sage: f = 1 + x + y^2
sage: type(f.degree())
<class 'sage.rings.integer.Integer'>
sage: type(f.degree(x))
<class 'sage.rings.integer.Integer'>
sage: type(f.degree(y))
<class 'sage.rings.integer.Integer'>
sage: type(f.total_degree())
<class 'sage.rings.integer.Integer'>
sage: type(f.degrees())
<class 'sage.rings.polynomial.polydict.ETuple'>
sage: type(f.degrees()[0])
<class 'int'>

I would like advice on how to proceed with the degrees() function. Is leaving it in it's current state OK?

Fixes #37603

@GiacomoPope GiacomoPope changed the title Ensure degree and total degree return Integer Ensure degree and total degree return Integer for MPolynomial_polydict type Mar 14, 2024
@GiacomoPope GiacomoPope changed the title Ensure degree and total degree return Integer for MPolynomial_polydict type Ensure degree and total degree return Integer type for MPolynomial_polydict class Mar 14, 2024
Copy link
Contributor

@grhkm21 grhkm21 left a comment

Choose a reason for hiding this comment

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

Just a small change.

For future reference, I considered moving the wrapper on L685 to the underlying PolyDict, but that is not desirable since polydict is a "general" dictionary-like class and hence should be fast.

src/sage/rings/polynomial/multi_polynomial_element.py Outdated Show resolved Hide resolved
@GiacomoPope
Copy link
Contributor Author

but that is not desirable since polydict is a "general" dictionary-like class and hence should be fast.

This is why I didnt make a patch for degrees() as well. I don't know what the right thing to do here is.

@GiacomoPope
Copy link
Contributor Author

Thanks!

Copy link

Documentation preview for this PR (built with commit a866b12; changes) is ready! 🎉

vbraun pushed a commit to vbraun/sage that referenced this pull request Mar 30, 2024
…s type `Integer` for `MPolynomialRing_libsingular` class

    
Previously the degree of multivariate polynomials was returned as a
python `int` instead of a SageMath `Integer` resulting in ugly
`ZZ(f.degree())` calls in various places.

This PR only focuses on the case of `MPolynomialRing_libsingular` to
keep the noise of the PR as low as possible.

**Future work**: This same issue is in `MPolynomial_polydict`, I am
happy to also do the work here, or make a new PR -- really I don't like
sage returning a `int` when we dont have to.

**Edit**

This follow up work has been done in PR:
sagemath#37611
    
URL: sagemath#37605
Reported by: Giacomo Pope
Reviewer(s): Matthias Köppe
@vbraun vbraun merged commit 7b6eb13 into sagemath:develop Mar 31, 2024
13 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Mar 31, 2024
@GiacomoPope GiacomoPope deleted the MPolynomial_polydict_integer_degree branch April 1, 2024 01:41
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.

Multivariate polynomial ring returns degree as an int
4 participants