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

Missing rounding modes? #81

Open
imirkin opened this issue Aug 7, 2023 · 6 comments
Open

Missing rounding modes? #81

imirkin opened this issue Aug 7, 2023 · 6 comments

Comments

@imirkin
Copy link

imirkin commented Aug 7, 2023

I was just playing with https://github.com/yaffle/BigDecimal to replace https://github.com/MikeMcl/big.js usage.

It seems like the proposed list in the README file, and the implementation above only provides floor/ceil, while big.js only provides up/down [in addition to half-up/half-down/etc]. The Java BigDecimal implementation of course has both sets.

I'd like to put in a good word for adding "up" and "down" to the roundingMode list. These are convenient for some financial applications, where negative/positive indicates whether one is long or short in a position, however the rounding is still expected to follow "positive" conventions, so rounding "up" would expect to make a number's magnitude larger. (Like let's say you're short 111234 shares, that's -111234. But then you want to see the number in thousands, so you want to see -112 rather than -111. And if you're long those same shares, you want to see 112 rather than 111. Or in some cases you do want the -111/111 in both cases. So up/down, not ceil/floor.)

In case there's any confusion, I'd expect:

let val = -5.1m;
expect(val.toFixed(0, "ceil")).toBe("-5");
expect(val.toFixed(0, "floor")).toBe("-6");
expect(val.toFixed(0, "up")).toBe("-6");
expect(val.toFixed(0, "down")).toBe("-5");

val = 5.1m;
expect(val.toFixed(0, "ceil")).toBe("6");
expect(val.toFixed(0, "floor")).toBe("5");
expect(val.toFixed(0, "up")).toBe("6");
expect(val.toFixed(0, "down")).toBe("5");
@jessealama
Copy link
Collaborator

My current thinking is that Decimal ought to provide the five rounding modes specified in IEEE 754:

  • round ties to even (default, same as in the various Math functions)
  • round ties up
  • round down
  • round up
  • round to zero

The round method would support all five, as would all mathematical operations and the constructor. How does that sound?

@ptomato
Copy link
Collaborator

ptomato commented Aug 30, 2023

I would suggest that it should provide the same set of rounding modes as NumberFormat and Temporal (expand, trunc, ceil, floor, halfExpand, halfTrunc, halfCeil, halfFloor, halfEven.) Four of these correspond to Math functions:

  • Math.round() - halfExpand (ties away from zero)
  • Math.trunc() - trunc (towards zero)
  • Math.ceil() - ceil (up)
  • Math.floor() - floor (down)
    I'd suggest that the default should be halfExpand since that's what many people mean colloquially when they say "rounding".

@imirkin
Copy link
Author

imirkin commented Aug 30, 2023

I'd like to second @ptomato's suggestion. IEEE 754 rounding makes sense for floating point contexts (maybe?), but for base-10 decimals, I think the Java BigDecimal is the gold standard -- other libraries tend to match it. You can find similar designs in decimal.js which is also used by many needing the functionality (or big.js as I do, which has less functionality, but by the same author). I'm not too fussed about the names, but there are reasons why these libraries support the rounding modes -- they're not hard to support, and each has its use-cases. Further, any logic which attempts to match what these do would greatly benefit from having this built-in.

I would also second the default mode being half-up (or halfExpand in @ptomato's comment). This is what e.g. decimal.js does. For the Java BigDecimal, a new MathContext defaults to HALF_UP if not supplied in the argument (although the DECIMAL32/64/128 variants are indeed HALF_EVEN to match IEEE 754R).

@jessealama
Copy link
Collaborator

Thanks! This is excellent feedback. It seems that there's a bit of a conflict here about which way to go:

  • Follow IEEE 754 and Math's lead and use half-to-even as the default.
  • Follow NumberFormat, Temporal, and various decimal JS libraries's lead and use half-expand as the default.

However, the signals point toward using half-expand as the default. The other options should at least exist, if one wishes to go against the default. (Supporting them is not difficult.)

@munrocket
Copy link

Crypto exchanges and banks will need banker rounding or in other words halfEven. Rationale behind this method is fairer and more accurate statistical representation.

Rounding mode in divide operation also needed? It will be in functional style or some state? For example new Decimal128('1').divide(new Decimal128('3')) will give infinite 0.3333333333333333333333334 with some rounding by default in the end.

@jessealama
Copy link
Collaborator

Rounding mode in divide operation also needed? It will be in functional style or some state? For example new Decimal128('1').divide(new Decimal128('3')) will give infinite 0.3333333333333333333333334 with some rounding by default in the end.

I'm not quite sure I understand. The intention is that all arithmetic operations take an optional argument (actually, an options bag) where one can specify the rounding mode. Perhaps I didn't document this correctly (or somehow missed divide)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants