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

Implement rounding mode for Number#round #10413

Merged

Conversation

straight-shoota
Copy link
Member

This patch adds Number#round(RoundingMode) which enables to chose rounding behaviour at runtime.

Supported rounding modes (per #10228 (comment)):

  • TO_ZERO
  • TO_POSITIVE
  • TO_NEGATIVE
  • TIES_EVEN
  • TIES_AWAY

Implementations were already available as dedicated methods for all modes except TIES_EVEN which is added as #round_even.

#round_away is the explicitly named implementation of TIES_AWAY mode previsouly just named #round.

#round without arguments now delegates to #round_away and the default rounding mode is TIES_AWAY for keeping backward compatibility.

Resolves #10228

There's already #7126 waiting with the implementation for big numbers (it will require some updates, though).

@bcardiff
Copy link
Member

Looks good. I would feel more comfortable with ceiling/floor. I think they are more familiar names. But I guess that sticking with IEEE-754 is fine.

src/float.cr Outdated Show resolved Hide resolved
@straight-shoota
Copy link
Member Author

The failing spec depends on #10413.

@bcardiff
Copy link
Member

I guess you meant #10420, right?

@bcardiff bcardiff added this to the 1.0.0 milestone Feb 19, 2021

# Specifies rounding behaviour for numerical operations capable of discarding
# precision.
enum RoundingMode
Copy link
Contributor

@Sija Sija Feb 21, 2021

Choose a reason for hiding this comment

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

I'd move it to the beginning of the file for a better discoverability.

Copy link
Member Author

Choose a reason for hiding this comment

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

What difference does it make? I think it should stay in proximity to the methods that use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could even consider extracting all features related to rounding into a separate file.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could even consider extracting all features related to rounding into a separate file.

Sounds even better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's leave that for a follow-up.

@HertzDevil
Copy link
Contributor

HertzDevil commented Apr 10, 2021

This breaks BigFloat:

require "big"

BigFloat.new("1.23").round # Error: undefined local variable or method 'round_away' for BigFloat

I'm not sure if anything can be done about this, because GMP doesn't implement TIES_AWAY and TIES_EVEN. MPFR does though (plus a whole bunch of other functions suitable for Math). We most certainly don't want to make the default something else only for BigFloats.

@straight-shoota
Copy link
Member Author

This is similar to #10599 and should probably be fixed by #7126.

@HertzDevil
Copy link
Contributor

HertzDevil commented Apr 10, 2021

It will not be fixed because GMP straight up doesn't define these rounding modes, and we cannot implement them for BigFloat on top of either BigDecimal or BigInt.

BigDecimal is, to my knowledge, not a drop-in replacement for BigFloat.

@straight-shoota
Copy link
Member Author

Oh, well. Then there is probably nothing we can do? The error is probably good then. You can't use BigFloat#round without rounding mode argument because the default mode is not implemented. We should make sure that using other rounding modes explicitly works, though.

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.

Add rounding mode for #round methods
4 participants