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

Allow negative shift for flint rational polynomial #39711

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

Conversation

user202729
Copy link
Contributor

Fixes #39710

Note that the behavior is compatible with the existing behavior of e.g. ZZ[].

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Mar 15, 2025

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

@tscrim
Copy link
Collaborator

tscrim commented Mar 17, 2025

I don't think this will (fully) fix #39710. If it indeed does, then please add the corresponding tests.

@user202729
Copy link
Contributor Author

Actually thinking about it this will still fail if user says something like .truncate_neg(-10^30) or .truncate_neg(10^30) because the computed value overflows. Even though .truncate_neg(-10^30) is completely safe, just return self.

On the other hand .truncate_neg(10^30) cannot work (the __n is a long so it would overflow n, unless the element has infinite precision in the beginning)

Actually it is worse than that: n - self.__n may overflow and return the wrong result?

Do you think it's worth supporting these (rather exceptional, I think) cases?

@tscrim
Copy link
Collaborator

tscrim commented Mar 17, 2025

I don't think it is worth worrying about those cases. I don't think there will be a good reason for someone to do computations like that.

Does this also work for (t+t^2).truncate_neg(-2)? i think you coincidentally get a shift by 0, which hides a bug that would delete coefficients.

@user202729
Copy link
Contributor Author

user202729 commented Mar 17, 2025

It does work. The point of >> is to delete coefficients (in order to truncate).

If n-self.__n is negative then it will shift left, no coefficient is deleted. (the two tests truncate_neg(-1) and truncate_neg(-2) have shift by -2 and -3 respectively.)

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.

Ah, you're right. I got the directions of everything with the signs all mixed up in my head. Thanks. Once tests pass, positive review.

From an optimization POV, I think it would be better to simply return self in the case n is less than the valuation, but that can be done on a followup (if you want).

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.

truncate_neg() fails on LaurentSeriesRing(QQ) when valuation is positive
2 participants