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

Add BigDecimal#round(digits, mode) overload #7126

Closed

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Nov 29, 2018

Fixes #5714

TBD:

  • BigDecimal.rounding_mode
  • BigDecimal.save_rounding_mode
  • BigDecimal.with_rounding_mode
  • Implementation review
  • Specs
  • Docs

src/big/big_decimal.cr Outdated Show resolved Hide resolved
@Sija Sija force-pushed the feature/bigdecimal-rounding-modes branch 2 times, most recently from 5f8824b to 589add7 Compare November 29, 2018 00:26
src/big/big_decimal.cr Outdated Show resolved Hide resolved
@Sija Sija force-pushed the feature/bigdecimal-rounding-modes branch 5 times, most recently from a2607dd to 003083c Compare November 29, 2018 16:15
@Sija Sija changed the title [WIP] Add BigDecimal#round(digits, mode) overload Add BigDecimal#round(digits, mode) overload Nov 29, 2018
@Sija Sija force-pushed the feature/bigdecimal-rounding-modes branch from a75179f to 1ea2fd8 Compare November 30, 2018 01:42
@Sija
Copy link
Contributor Author

Sija commented Nov 30, 2018

Ready for review.

@Sija
Copy link
Contributor Author

Sija commented Dec 3, 2018

@RX14 @bcardiff @asterite would you mind a rrrreview?

@RX14
Copy link
Contributor

RX14 commented Dec 6, 2018

This doesn't even attempt to be compatible with Number#round. It also seems completely unoptimized, have you looked at what other implementations do here?

@Sija
Copy link
Contributor Author

Sija commented Dec 6, 2018

This doesn't even attempt to be compatible with Number#round.

That's true, some goes for Float{32,64}#round and Int#round (they handle neither ndigits, nor base arguments) - it would be nice to sort 'em out, but not before having uniformly working mode argument - which would be better done in a separate PR.

Ruby has more uniform API in this regard, with #round([ndigits][, mode]) signature.

It also seems completely unoptimized, have you looked at what other implementations do here?

Unfortunately, that's quite likely, but you're free to post your optimization suggestions, let's treat it as yet another Advent of Code puzzle ;)

@RX14
Copy link
Contributor

RX14 commented Dec 7, 2018

I don't want to merge this before we sort out a rounding API on Number and make Int and Float follow it. Then we can make this PR follow that API too.

@Sija
Copy link
Contributor Author

Sija commented Dec 7, 2018

That's true, some goes for Float{32,64}#round and Int#round (they handle neither ndigits, nor base arguments) - it would be nice to sort 'em out, but not before having uniformly working mode argument - which would be better done in a separate PR.

I'd like to correct my statement, since Float{32,64}#round and Int#round are more like optimizations of Number#round, with exception of missing handling base argument; btw, is it really needed? Aside of digits and sometimes mode, I haven't seen much of it in stdlibs of other languages.

Now, as for cleaning up the API, I'd suggest dropping base argument, would leave us with consistent API of Float, Int and Number.

Additional (named?) argument mode support would stay for BigDecimal only. Would be nice to have it working for Number#round, but to my (limited) understanding, that would mean loosing Float#round optimization when mode argument is used.

@Sija
Copy link
Contributor Author

Sija commented Dec 12, 2018

@RX14 any thoughts?

@RX14
Copy link
Contributor

RX14 commented Dec 21, 2018

Same as before, we need to unify the API. We should keep base.

@Sija
Copy link
Contributor Author

Sija commented Dec 21, 2018

@RX14 yeah, you said it before, any (counter) suggestions how? and why in your opinion should we keep base?

@Sija
Copy link
Contributor Author

Sija commented Mar 7, 2019

Hi, what's the status of this PR?

@RX14 proposed unification of Number rounding API stating that:

[...] We should keep base.

I disagree, personally I've never had a need to specify base while rounding, neither I've seen it anywhere in the wild. Ruby, JavaScript, Python, PHP - they all have pretty consistent APIs regarding rounding numbers, in which base is nowhere to be seen.

In terms of Number#round API unification I'd rather vote for removal of before-mentioned base argument.

BigDecimal#round is especially useful for money-related shards, thus its absence hurts stdlib functionality and BigDecimal feature set.

I'd like to continue working on it once some course of action is settled.

@Sija
Copy link
Contributor Author

Sija commented Jul 7, 2020

Mornin' ☀️ !

I've force-pushed refactored implementation which, I believe should be a tad more performant, and so I'd like to revisit this PR if possible since this feature comes rrrly handy for these few occasions (think financial, especially crypto), thanks for the attention and have a great day there :) 🙏

Side note: Some of the GHA CI jobs (CircleCI is ✅ ) seem to use a stale cache or sth - the spec failures doesn't seem to cover for the actual code - that or... might a be a red herring since it's already just a bit l8 for me - or early for that matter 🙃

@Sija Sija force-pushed the feature/bigdecimal-rounding-modes branch 2 times, most recently from f761e16 to 99f2f52 Compare July 8, 2020 00:23
@Sija
Copy link
Contributor Author

Sija commented Jul 9, 2020

ping, could this get a review please? /cc @straight-shoota @asterite @waj

@asterite
Copy link
Member

asterite commented Jul 9, 2020

@Sija I don't think this has any pressure to get into 1.0. There are a bunch of more important things right now to make sure 1.0 is properly released, so please be patient until we get some time to review this and other PRs. Thank you!

@straight-shoota
Copy link
Member

This can now be refactored on top of #10413.

@straight-shoota
Copy link
Member

I don't think we should add the global rounding_mode property. Or at least, it should be a different discussion. This should focus on implementing the missing rounding methods for use with Number#round. That is (probably?) just round_away and round_even.

It might actually be best to close this and start with a fresh PR.

@Sija Sija force-pushed the feature/bigdecimal-rounding-modes branch from 77c088a to e942306 Compare March 19, 2021 12:39
@Sija
Copy link
Contributor Author

Sija commented Mar 19, 2021

@straight-shoota rebased on top of the current master.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Please exclude rounding_mode setting from this PR. This feature warrants a dedicated discussion. If at all, it should probably be global to Number and not specific to BigDecimal (see #10414 (comment) ff.).

Also BigDecimal is missing the basic rounding mode methods #round_away and #round_even.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Misclicked on Approve

@straight-shoota
Copy link
Member

@Sija Are you up to address the last review commment and remove rounding_mode, add missing methods?

@Sija
Copy link
Contributor Author

Sija commented Apr 13, 2021

@straight-shoota Will do, sadly I'm not sure I can give you any sensible timeframe for that.

in .to_negative? then negative
end

value += 1 if round_up
Copy link
Contributor

Choose a reason for hiding this comment

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

This addition is incorrect, since value is already the BigDecimal itself, not the mantissa:

BigDecimal.new("123.456789").round(2, mode: :ties_away) # => 124.45

@HertzDevil
Copy link
Contributor

With #10798 merged and the part about BigDecimal.rounding_mode deferred, this PR can probably be closed.

@asterite asterite closed this Jun 18, 2021
@straight-shoota
Copy link
Member

Thank you both, @Sija and @HertzDevil =)

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.

Implement rounding modes for BigDecimal
7 participants