Skip to content

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

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
Merged
merged 4 commits into from
Mar 31, 2024
Merged

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

merged 4 commits into from
Mar 31, 2024

Conversation

GiacomoPope
Copy link
Contributor

@GiacomoPope GiacomoPope commented Mar 14, 2024 •
edited
Loading

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 mentioned this pull request Mar 14, 2024
Merged
@GiacomoPope GiacomoPope requested a review from mkoeppe March 14, 2024 11:01
@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
grhkm21
grhkm21 suggested changes Mar 14, 2024
View reviewed changes
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.

@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!

@github-actions GitHub Actions
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
3ee8402 
sagemathgh-37605: Ensure returned degree of multivariate polynomial is 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
@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
Reviewers

@mkoeppe mkoeppe Awaiting requested review from mkoeppe

1 more reviewer

@grhkm21 grhkm21 grhkm21 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
c: commutative algebra p: minor / 4 t: refactoring
Projects
None yet
Milestone
sage-10.4
Development

Successfully merging this pull request may close these issues.

Multivariate polynomial ring returns degree as an int
4 participants
@GiacomoPope @grhkm21 @vbraun @mkoeppe