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

Finish changing Rational's round method default rounding to even #39486

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

Conversation

user202729
Copy link
Contributor

Follow-up to #35756 .

(As is, the warning cannot be suppressed)

Related: #37306, #21935

There's a little (large??) problem: previously K2.<sqrt2> = QuadraticField(2); K2(9/2).round() returns 5 without a warning (because #35756 forget to print the deprecation). Should we add the deprecation first then change this next year?

📝 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

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

@vneiger
Copy link
Contributor

vneiger commented Feb 10, 2025

In fact, I would say the deprecation acts as intended, cf DeprecationWarning :
Base category for warnings about deprecated features when those warnings are intended for other Python developers (ignored by default, unless triggered by code in main).

Therefore, since it has been about 18 months since the deprecation introduced in #35756, it could indeed be the right time to actually make the change (from default away to even).

@vneiger
Copy link
Contributor

vneiger commented Feb 10, 2025

It seems to me that the failing doctests are not linked to the changes here (one I cannot reproduce, and the other one I can reproduce but already without the changes of this PR). The changes look good to me.

@vneiger
Copy link
Contributor

vneiger commented Feb 10, 2025

Another set of eyes may be useful before a positive review. Maybe @mezzarobba who had also contributed to #35756 ?

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.

2 participants