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

Support scientific notation in BigDecimal#to_s #10805

Merged

Conversation

HertzDevil
Copy link
Contributor

Resolves #9578.

# before
BigDecimal.new(Float32::MAX)          # => 340282350000000000000000000000000000000
BigDecimal.new(Float32::MIN_POSITIVE) # => 0.000000000000000000000000000000000000011754944

# after
BigDecimal.new(Float32::MAX)          # => 3.4028235e+38
BigDecimal.new(Float32::MIN_POSITIVE) # => 1.1754944e-38

The code here is very similar to #10632 and Float::Printer#internal. If both are merged we should think about refactoring them into a single implementation.

src/big/big_decimal.cr Outdated Show resolved Hide resolved
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.

This doesn't break any formal API, but it's still a significant change in behaviour. I you expect ˋ0ˋ to stringify as ˋ0ˋ, returning ˋ0.0ˋ breaks that.

So... do we have to wait for 2.0 with this?

buffer = Slice.new(cstr, length)

# add negative sign
if buffer[0]? == 45 # '-'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if buffer[0]? == 45 # '-'
if buffer[0]? === '-'

I'm pretty sure this doen't add any significant overhead and saves you from annotating the character in a comment ;)

Copy link

Choose a reason for hiding this comment

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

“===” different from ruby syntax ?

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Jul 23, 2021

Some additional clarification: unlike #10632 (comment), fixed precision formatting is actually possible for BigDecimals converted from Floats, but the current constructor doesn't work because it relies on Float#to_s, and any BigDecimals created this way are inherently lossy. Instead the float must be decomposed manually:

require "big"

struct BigDecimal
  def **(other : Int) : BigDecimal
    return (to_big_r ** other).to_big_d if other < 0 # #10892
    BigDecimal.new(@value ** other, @scale * other)
  end

  def self.exact(float : Float32)
    mantissa, exponent = Math.frexp(float)
    Math.ldexp(mantissa, Float32::MANT_DIGITS).to_i * 2.to_big_d ** (exponent - Float32::MANT_DIGITS)
  end

  def self.exact(float : Float64)
    mantissa, exponent = Math.frexp(float)
    Math.ldexp(mantissa, Float64::MANT_DIGITS).to_i * 2.to_big_d ** (exponent - Float64::MANT_DIGITS)
  end
end

# Before:
BigDecimal.exact(Float32::MAX)          # => 340282346638528859811704183484516925440
BigDecimal.new(Float32::MAX)            # => 340282350000000000000000000000000000000
BigDecimal.exact(Float32::MIN_POSITIVE) # => 0.0000000000000000000000000000000000000117549435082228750796873653722224567781866555677208752146087936
BigDecimal.new(Float32::MIN_POSITIVE)   # => 0.000000000000000000000000000000000000011754944

# After:
BigDecimal.exact(Float32::MAX)          # => 3.4028234663852885981170418348451692544e+38
BigDecimal.new(Float32::MAX)            # => 3.4028235e+38
BigDecimal.exact(Float32::MIN_POSITIVE) # => 1.17549435082228750796873653722224567781866555677208752146087936e-38
BigDecimal.new(Float32::MIN_POSITIVE)   # => 1.1754944e-38

Like the corresponding PR for BigFloat, this patch never changes the number of significant digits in the output of BigDecimal#to_s.

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

I don't think there were complains about pushing #10632, so I think we can follow suit here. Sorry Quinton that it took this long, and thanks! 🙇

src/big/big_decimal.cr Outdated Show resolved Hide resolved
@beta-ziliani beta-ziliani added this to the 1.7.0 milestone Nov 22, 2022
Co-authored-by: Beta Ziliani <beta@manas.tech>
@straight-shoota straight-shoota merged commit 022e1f9 into crystal-lang:master Nov 24, 2022
@HertzDevil HertzDevil deleted the feature/bigdecimal-scientific branch November 26, 2022 06:22
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.

RFC: change default string representation in BigDecimal
5 participants