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

Fix #55820 : missing _convert_rounding methods for RoundFromZero #57802

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

RafaelAvelar14
Copy link

  • Implemented _convert_rounding method for RoundingMode{:FromZero} to handle conversions to AbstractFloat.
  • Supported conversions for the following floating-point types:
    • Float16, Float32, Float64, AbstractFloat, BigFloat
  • Handled real number types including:
    • Bool, Rational{Int8}
  • Added tests to validate conversions for all affected type combinations.

…Zero

- Implemented `_convert_rounding` method for `RoundingMode{:FromZero}`
  to handle conversions to `AbstractFloat`.
- Supported conversions for the following floating-point types:
  - Float16, Float32, Float64, AbstractFloat, BigFloat
- Handled real number types including:
  - Bool, Rational{Int8}
- Added tests to validate conversions for all affected type
  combinations.
Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! It's lovely. I have a few comments but this should be pretty easy to merge. And welcome! Please let me know if you have any questions or need help contributing.

RafaelAvelar14 and others added 2 commits March 17, 2025 18:35
Renamed test to better reflect its purpose and included the issue number for easier tracking.

Co-authored-by: Lilith Orion Hafner <[email protected]>
- Improve test coverage beyond zero-to-zero cases
- Include tests for large positive and negative values  
- Ensure correct rounding behavior for nextfloat(0.0)

Co-authored-by: Lilith Orion Hafner <[email protected]>
@RafaelAvelar14
Copy link
Author

Hey @LilithHafner, I've tested and committed the suggested tests. Let me know if there's anything else to adjust. Thanks for the review.

@LilithHafner
Copy link
Member

Thanks! Please also remove as many deviations from the ToZero version as possible.

@LilithHafner
Copy link
Member

While you are welcome to use AI to assist you in learning or explaining things to you, it is not acceptable to post AI generated content under your own name or otherwise under the guise of human content.

@RafaelAvelar14
Copy link
Author

@LilithHafner

I'm sincerely sorry, I’ve deleted the comment and I won’t use AI for writing assistance anymore. It was mainly for grammar correction. The code and tests are entirely my own, would it still be okay to proceed with the commit?

@LilithHafner
Copy link
Member

No, we can't proceed with merging until you address #57802 (comment) and #57802 (comment)

@LilithHafner LilithHafner added the merge me PR is reviewed. Merge when all tests are passing label Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me PR is reviewed. Merge when all tests are passing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants