Skip to content

fix gcd, coefficient and monomial_coefficient #40517

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

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

mantepse
Copy link
Contributor

@mantepse mantepse commented Jul 31, 2025

fix #40504

This is only patching the bugs. A more thorough redesign would be desirable, but is too much work for me right now.

In particular, I think that implementing __getattr__ to magically translate methods to the underlying polynomial ring is a bad idea.

@mantepse mantepse requested a review from tscrim July 31, 2025 16:41
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

A few small things; otherwise LGTM.

"""
try:
return InfinitePolynomial_sparse(self.parent(), self._p.gcd(x._p))
except Exception:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we be more specific about what type of exception this will raise (e.g., ValueError, TypeError?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this code is rather bad anyway, I only kept it to keep it consistently so.

If you want to, I would replace all the occurrences (not only in gcd) with

        try:
            result = self._p.gcd(x._p)
        except TypeError:
            ...  # compute result by first creating the correct parent
        return InfinitePolynomial_sparse(self.parent(), result)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I just decided to factor out the logic that creates a common ring for the argument and self. This makes the code much more readable.

Copy link

github-actions bot commented Aug 4, 2025

Documentation preview for this PR (built with commit 55d396f; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

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.

illegal coercion between polynomial rings in InfinitePolynomial.coefficient
2 participants