Implement BigDecimal #4876

vegai · 2017-08-23T05:28:08Z

This PR adds a BigDecimal implementation.

The motivation for having BigDecimals is to have an arbitrary precision exact decimal type. This is required at least in financial calculations, where the precision of Floats and is not enough and BigRationals may have other problems due to denominator potentially being something else than 10**x. At least Ruby, Python and Java contain something like this in their standard libs.

The design is mostly adapted from bigdecimal-rs written for Rust. It holds its actual value in a BigInt, and a scale (decimal point place) in UInt64. It currently contains basic + - * / arithmetics. Possible future needs: implementation of ** (power) and supporting scientific "123E45" notation.

Please critique liberally. This would be my first larger Crystal contribution.

Sija · 2017-08-23T14:42:25Z

src/big/big_decimal.cr

+  # Set maximum iterations used in division operation. This implicitly
+  # defines the maximum precision of divisions in cases where the
+  # division is not exact.
+  def self.set_max_div_iterations(x : Int)


Please use descriptive names for arguments.

=> new_max_div

Sija · 2017-08-23T14:42:30Z

src/big/big_decimal.cr

+  # Create a new `BigDecimal` from a String.
+  #
+  # Allows only valid number strings with an optional negative sign.
+  def initialize(s : String)


Use ditto to use the same comment as in the previous declaration.

https://crystal-lang.org/docs/conventions/documenting_code.html

=> str

@makenowjust I guess this comment didn't apply here?

@makenowjust Quoted remark applies to documentation, not code-review comments, mind you.

Sija · 2017-08-23T14:42:35Z

src/big/big_decimal.cr

+  end
+
+  # from Int
+  def initialize(i : Int)


Sija · 2017-08-23T14:42:41Z

src/big/big_decimal.cr

+  end
+
+  # from Float is not supported due to precision loss risks. This call fails at compile time.
+  def initialize(f : Float)


Sija · 2017-08-23T14:43:53Z

src/big/big_decimal.cr

+  end
+
+  private def power_ten_to(x : Int) : Int
+    BigInt.new(10) ** x


Sija · 2017-08-24T11:01:11Z

src/big/big_decimal.cr

+end
+
+struct Float
+  # from Float is not supported due to precision loss risks. This call fails at compile time.


I'd use full sentence here, like: "Casting from Float is not supported due to ..."

So changed.

@vegai you should use ` around Float in your comment: Casting from `Float` ...

Marked this Float and all the other similar ones too.

akzhan · 2017-08-25T13:19:51Z

src/big/big_decimal.cr

+  include Comparable(BigDecimal)
+
+  # Convert `Int` to `BigDecimal`
+  def to_big_d


Looks like you forget to declare

struct BigDecimal def to_big_d self end end

to optimize BigDecimal case of to_big_d (we need no copy here).

Yes. Added.

Sija · 2017-08-25T13:45:17Z

src/big/big_decimal.cr

+    elsif @scale > s.size
+      io << "0."
+      (@scale - s.size).times do
+        io << "0"


You could use a Char here.

No, string is better here because need no conversion from Char to utf8 bytes.

@Sija I'm wrong! Unbelievable!

➜ crystal git:(bigfloat-frexp) ✗ bin/crystal 1.cr --no-debug --release Using compiled compiler at `.build/crystal' char 370.13 ( 2.7ms) (± 0.91%) fastest string 150.32 ( 6.65ms) (± 1.97%) 2.46× slower byte from char 12.61 ( 79.32ms) (± 2.65%) 29.36× slower byte from string 133.63 ( 7.48ms) (± 1.40%) 2.77× slower byte 132.72 ( 7.53ms) (± 1.82%) 2.79× slower

require "benchmark" Benchmark.ips do |x| x.report("char") { io = IO::Memory.new; 1_000_000.times { io << '.' } } x.report("string") { io = IO::Memory.new; 1_000_000.times { io << "." } } x.report("byte from char") { io = IO::Memory.new; 1_000_000.times { io << '.'.bytes } } x.report("byte from string") { io = IO::Memory.new; 1_000_000.times { io << ".".byte_at(0) } } x.report("byte") { b = ".".byte_at(0); io = IO::Memory.new; 1_000_000.times { io << b } } end

@vegai Could you please include above tweaks?

Huh, interesting :) Seems almost like a performance bug somewhere.

But will change these to chars then.

Sija · 2017-08-25T13:45:24Z

src/big/big_decimal.cr

+      io << s
+    else
+      offset = s.size - @scale
+      io << s[0...offset] << "." << s[offset..-1]


akzhan · 2017-08-26T15:08:42Z

LGTM

RX14 · 2017-08-27T06:31:25Z

spec/std/big/big_decimal_spec.cr

+
+    (BigDecimal.new(1) > BigDecimal.new(1)).should be_false
+    (BigDecimal.new("1.00000000000000000000000000000000000001") > BigDecimal.new(1)).should be_true
+    (BigDecimal.new("0.99999999999999999999999999999999999999") > BigDecimal.new(1)).should be_false


You should test less than here.

Added < tests

(BigDecimal.new("0.99999999999999999999999999999999999999") < BigDecimal.new(1)).should be_true

RX14 · 2017-08-27T06:33:32Z

spec/std/big/big_decimal_spec.cr

+    (BigDecimal.new("-1") < BigDecimal.new("1")).should be_true
+  end
+
+  it "arithmetic that beats float precision" do


it "keeps precision"? In general these it names don't flow well.

Yeah, I wasn't thinking at all there. Changed all the descriptions.

RX14 · 2017-08-27T06:38:30Z

src/big/big_decimal.cr

+
+  include Comparable(BigDecimal)
+  include Comparable(Int)
+  include Comparable(String)


We shouldn't be comparable with string (at least I don't think any other numeric is) but we should be comparable with all other numbers not just int. We shouldn't need multiple comparable includes here, just Comparable(Numeric).

Replaced Int and String with Comparable(Number)

Oops, misremembered the name. Thanks!

RX14 · 2017-08-27T06:40:36Z

src/big/big_decimal.cr

+struct BigDecimal
+  ZERO = BigInt.new(0)
+  TEN  = BigInt.new(10)
+  @@max_div_iterations = 100_u64


Please use class_property. I'm not entirely sure I like that this is global, maybe we should make it configurable per div call with the default being this property.

Yeah, I thought it was a bit awkward too. The original algorithm in Rust just had this value hardcoded inside the div method. I'll take a look at configuring it per div call. I cannot make this an optional parameter of / though, can I?

Made it configurable per div call as suggested: added a div function with the optional parameter, and made / call that one. DEFAULT_MAX_DIV_ITERATIONS is now a class constant.

RX14 · 2017-08-27T06:42:51Z

src/big/big_decimal.cr

+
+  private property value : BigInt
+  private property scale : UInt64
+  getter value, scale


Please don't do this. Use a public getter and simply use @value when you want to set.

For that matter, you should never need to set these values because this struct should be immutable.

Hmm, I didn't quite get the gist here. Doesn't this combo make the value immutable from the outside world? Or does Crystal have better immutability protections that I've missed?

RX14 · 2017-08-27T07:54:19Z

src/big/big_decimal.cr

+    check_division_by_zero other
+
+    scale = @scale - other.scale
+    n, d = @value, other.@value


Please don't use single character variable names.

RX14 · 2017-08-27T07:55:00Z

src/big/big_decimal.cr

+    scale = @scale - other.scale
+    n, d = @value, other.@value
+
+    quotient, remainder = n.tdiv(d), n.remainder(d)


Could you use divmod here?

They're not quite the same. divmod floors negative integers down, which doesn't work here without additional changes.

1.divmod(-2) => {-1, -1} 1.tdiv(-2) => 0 1.remainder(-2) => 1

But I'll think a bit if it would be a good idea to adapt the algo to divmod.

Huh, unearthed a bug here. -1 / -2 gives us -0.5 currently.

Fixed bugs, added bunch of div tests, and moved to divmod.

RX14 · 2017-08-27T07:57:21Z

src/big/big_decimal.cr

+  end
+
+  # from `Int`
+  def initialize(num : Int)


There should be a far faster way to initialize from integers than via to_s.

Oh, whoops. Somebody was lazy :P Fixed.

RX14 · 2017-08-27T07:58:56Z

src/big/big_decimal.cr

+    remainder = remainder * TEN
+
+    i = 0
+    while remainder != ZERO && i < @@max_div_iterations


Shouldn't you raise if you reach max_div_iterations, not give the wrong result.

It's not very exceptional, since simple calculations like 1/3 hit this.

Oh never mind then. I didn't read the code properly.

RX14 · 2017-08-27T08:01:49Z

src/big/big_decimal.cr

+struct Float
+  # Casting from `Float` is not supported due to precision loss risks. This call fails at compile time.
+  def to_big_d
+    {% raise "Initializing from Float is risky due to loss of precision -- convert rather from Int or String" %}


Same as before, I guess?

Sija · 2017-08-27T18:25:02Z

spec/std/big/big_decimal_spec.cr

+  end
+
+  it "keeps precision" do
+    oneThousandth = BigDecimal.new("0.001")


please, use snake_case for variable names; we ain't in JavaScript Land Dorothy ;)

Heh. Fixed.

Sija · 2017-08-28T19:39:44Z

src/big/big_decimal.cr

-    return BigDecimal.new(quotient, scale) if remainder == ZERO
+    # quotient, remainder = n.tdiv(d), n.remainder(d)
+    quotient, remainder = n.divmod(d)
+    puts "n #{n} d #{d} q #{quotient} r #{remainder}"


Debug leftovers.

Yeah, intermediate commits to save work. Removed afterwards

RX14 · 2017-08-28T21:09:53Z

src/big/big_decimal.cr

+struct BigDecimal
+  ZERO                       = BigInt.new(0)
+  TEN                        = BigInt.new(10)
+  DEFAULT_MAX_DIV_ITERATIONS = 100_u64


Personally I wouldn't bother making this a constant but it's not worth any effort making it not a constant.

So ignore this comment.

akzhan · 2017-09-06T23:54:38Z

Looks like You forgot about

struct BigDecimal
  # Returns *num*. Useful for generic code that does `T.new(...)` with `T`
  # being a `Number`.
  def self.new(num : BigDecimal)
    num
  end

Some info here: #2292.

vegai · 2017-09-07T10:58:03Z

@akzhan Thanks, added that.

vegai · 2017-10-16T09:32:51Z

Added to_f/u/i implementations and a few specs. to_f goes via to_s while to_u and _i scale and truncate like Java's implementation.

Also rebased against master and changed require "big_int" to require "big".

RX14 · 2017-10-16T14:41:51Z

src/big/big_decimal.cr

+      (@value / TEN ** @scale)
+    else
+      -(@value.abs / TEN ** @scale)
+    end.to_i


No, we never chain a method call after end. The correct way to write this is to stick the to_i on the end of the if branches, which works because of explicit return.

If this wasn't the final (only) statement in the method, we simply assign to a variable inside if:

if @value >= 0 int_value = (@value / TEN ** @scale).to_i else int_value = -(@value.abs / TEN ** @scale).to_i end # use int_value

Righto. Fixed.

vegai · 2017-11-06T08:13:19Z

Should this PR be in the "Numbers" project?

Sija · 2017-11-06T13:11:56Z

src/big/big_decimal.cr

+  # defines a maximum number of iterations in case the division is not exact.
+  #
+  # ```
+  # BigDecimal(1).div(BigDecimal(2)) => BigDecimal(@value=5, @scale=2)


Missing # before =>. On the line below as well.

Roger, fixed.

Sija · 2017-11-06T13:12:16Z

src/big/big_decimal.cr

+    div other
+  end
+
+  # Divides self with another `BigDecimal`, with a optionally configurable max_div_iterations, which


max_div_iterations -> *max_div_iterations*

Sija · 2017-11-06T13:13:01Z

src/big/big_decimal.cr

+    hasher.string(self.to_s)
+  end
+
+  # Returns the quotient as absolutely negative if self and other have different signs,


quotient -> *quotient*

Fixed (also line below).

Thanks!

onemanstartup · 2017-11-07T13:07:00Z

Thank you! 🎉

mverzilli · 2017-11-07T13:15:21Z

Thank you @vegai! Amazing work, and outstanding patience to bear with reviews :)

akzhan · 2017-11-07T13:29:42Z

src/big/big_decimal.cr

+  end
+
+  def hash(hasher)
+    hasher.string(self.to_s)


Of course it must be rewritten using number normalization.

But it will be anyway later.

Sija · 2017-11-07T13:31:51Z

This PR added src/big_decimal.cr which got obsolete because of #5121.

Sija · 2017-11-07T14:15:08Z

btw, why BigDecimal doesn't inherit from Number?

akzhan · 2017-11-12T16:52:56Z

@oprypin Done - #5276

Sija reviewed Aug 23, 2017

View reviewed changes

vegai force-pushed the bigdecimal branch 3 times, most recently from def36c3 to 6e9a815 Compare August 24, 2017 05:36

Sija reviewed Aug 24, 2017

View reviewed changes

vegai force-pushed the bigdecimal branch from 84c156c to 07bff5b Compare August 25, 2017 06:14

akzhan suggested changes Aug 25, 2017

View reviewed changes

Sija reviewed Aug 25, 2017

View reviewed changes

vegai force-pushed the bigdecimal branch from db49045 to a284955 Compare August 26, 2017 14:31

RX14 requested changes Aug 27, 2017

View reviewed changes

vegai force-pushed the bigdecimal branch 2 times, most recently from b8b2025 to 8a73f37 Compare August 27, 2017 13:13

Sija reviewed Aug 27, 2017

View reviewed changes

vegai force-pushed the bigdecimal branch 2 times, most recently from e1c41d9 to d3728f0 Compare August 28, 2017 12:03

Sija reviewed Aug 28, 2017

View reviewed changes

RX14 reviewed Aug 28, 2017

View reviewed changes

vegai force-pushed the bigdecimal branch 3 times, most recently from 5470f7b to 9346020 Compare August 28, 2017 21:44

vegai force-pushed the bigdecimal branch 2 times, most recently from e050e69 to 1b202bb Compare September 7, 2017 10:57

vegai added 6 commits October 16, 2017 11:26

Change require big_int to big as per crystal-lang#5121

9bf8f89

Add a failing to_s test

c2e7846

Fix to_s on negative numbers > -1.0

a1fabe4

Add failing to_i/u/f conversion specs

3758be1

Fix few to_s edge cases. Implement to_f/i/u

9cb9664

More concise hash invariant check

d80a81b

Document to_i and to_u's truncation behaviour

10e9a05

RX14 reviewed Oct 16, 2017

View reviewed changes

Don't call to_i on a block

e761944

RX14 approved these changes Oct 16, 2017

View reviewed changes

Combine redundant branches away

70b2a7b

Sija reviewed Nov 6, 2017

View reviewed changes

CR fixes

3a36303

mverzilli merged commit 1ccd22b into crystal-lang:master Nov 7, 2017

mverzilli added this to the Next milestone Nov 7, 2017

akzhan reviewed Nov 7, 2017

View reviewed changes

Sija mentioned this pull request Nov 7, 2017

Implement BigDecimal followup #5255

Merged

vegai deleted the bigdecimal branch November 8, 2017 07:02

akzhan mentioned this pull request Nov 12, 2017

Number normalization for Crystal::Hasher #5276

Merged

Sija mentioned this pull request Jan 2, 2018

BigDecimal enhancements #5390

Merged

bcardiff mentioned this pull request Feb 12, 2020

Fix BigDecimal#to_big_i regression #8790

Merged

Implement BigDecimal #4876

Implement BigDecimal #4876

Conversation

vegai commented Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akzhan Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akzhan Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akzhan commented Aug 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegai Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegai Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegai Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegai Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegai Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akzhan commented Sep 6, 2017

vegai commented Sep 7, 2017

vegai commented Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegai commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sija Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegai commented Aug 23, 2017 •

edited

Loading

akzhan Aug 25, 2017 •

edited

Loading

akzhan Aug 25, 2017 •

edited

Loading

vegai Aug 27, 2017 •

edited

Loading

vegai Aug 28, 2017 •

edited

Loading

vegai Aug 27, 2017 •

edited

Loading

vegai Aug 28, 2017 •

edited

Loading

vegai Aug 28, 2017 •

edited

Loading

vegai commented Oct 16, 2017 •

edited

Loading

Sija Nov 6, 2017 •

edited

Loading