Additions to Big arithmetics #4653

akzhan · 2017-07-01T15:26:02Z

Math.frexp(BigFloat) added.
BigRational.to_big_f and BigInt.to_big_f added.
BigFloat.trunc added with specs that shows difference with floor.
More number converters and constructors.
BigFloat./(Int32/Int64) specialization.
BigInt.divmod specialization.

Follows #4560.

This pull request includes changes required to support number normalization for #4675.

funny-falcon · 2017-07-06T10:56:36Z

src/big/big_float.cr

+module Math
+  def frexp(value : BigFloat)
+    frac = LibGMP.mpf_get_d_2exp(out exp, value)
+    {frac, exp}


Implementation for exact calculation (for example, for exact hashing) should look like

module Math def frexp(value : BigFloat) LibGMP.mpf_get_d_2exp(out exp, value) frac = BigFloat.new { |mpf| if exp >= 0 LibGMP.mpf_div_2exp(mpf, self, exp) else LibGMP.mpf_mul_2exp(mpf, self, -exp) end } {frac, exp} end end

Thanks, will do

funny-falcon · 2017-07-06T16:39:53Z

src/big/big_int.cr

+
+module Math
+  def frexp(value : BigInt)
+    frac = LibGMP.get_d_2exp(out exp, value)


I'd rather fallback to frexp(BigFloat.new(value)).

Just thinking about -)

funny-falcon · 2017-07-07T06:07:09Z

I've made BigFloat.new(BigInt) and BigFloat.new(BigRational) as a part of #4675.

funny-falcon · 2017-07-07T06:10:26Z

(but i forgot to add BigInt#to_big_f and BigRational#to_big_f)

I mean: there will be collisions between this PR and #4675.

akzhan · 2017-07-07T08:14:28Z

It's ok. And to_big_f is crystal way.

akzhan · 2017-07-07T08:16:47Z

May be we need to copy to_big_f to your PR, because it's usage looks more clear.

funny-falcon · 2017-07-07T08:45:34Z

I found mpf_set_q doesn't do what most expected from.
I mean: BigRational.new(a,b).to_big_f != BigFloat.new(a)/BigFloat.new(b)
Test with a=10000000000000001;b=10000000000000000

akzhan · 2017-07-07T08:55:28Z

require "big"

def tst(a, b)
 p BigRational.new(a,b).to_big_f
 p BigFloat.new(a) / BigFloat.new(b)
end

tst(10000000000000001, 10000000000000000)

➜  crystal git:(bigfloat-frexp) ✗  bin/crystal test.cr
Using compiled compiler at `.build/crystal'
1.0000000000000001
1

to_big_f works fine. But BigFloat./ doesn't have good precision.

funny-falcon · 2017-07-07T09:06:15Z

Yes, it looks like my version of BigFloat.new(BigRational) has a mistake.
Can you look at, please?
Your BigRational#to_big_f is correct. I don't understand where is difference.

funny-falcon · 2017-07-07T09:09:19Z

I see: compiler ignores my BigFloat.new(BigRational) and calls BigFloat.new(Number) :-(

funny-falcon · 2017-07-07T09:21:51Z

Fixed : it was need to require "big/big_rational" for some reason.

Note: if you do BigFloat constructors for every type (like in my PR), then there will be no need to add #to_big_f for every type, cause Number#to_big_f should do the thing.

akzhan · 2017-07-07T09:29:29Z

Thanks, @funny-falcon for clarification but this is strange. Anyway I'll try to copy GMP related logic here to simplify any merge etc.

akzhan · 2017-07-07T09:43:15Z

Hmm, dropping of to_big_f for BigInt throws bug. Kinda keeping of to_big_f did behavior predictable.

  1) BigInt does to_big_f
     Failure/Error: a.to_big_f.should eq(BigFloat.new("1234567890123456789.0"))

       Expected: 1234567890123456789
            got: 1234567890123456768

     # spec/std/big/big_int_spec.cr:227

The ASTNode of `1 <= 2 <= 3` is `to_s`-ed to `(1 <= 2) <= 3`. Right side parenthesis is not required and it causes crystal-lang#4653 error.

RX14 · 2017-07-14T16:05:37Z

@akzhan build is failing because of formatter.

akzhan · 2017-07-14T17:23:46Z

@RX14 Fixed, thanks.

Sija · 2017-07-14T20:47:37Z

src/big/big_float.cr

@@ -1,5 +1,6 @@
 require "c/string"
 require "./big"
+require "./big_rational" # Strange but required for working BigFloat.new(BigRational) on Travis CI.


It looks like it didn't help :/

Bad kind of magic was restored not exactly as @funny-falcon proposed. Just updated (сross fingers). Will create issue for this.

akzhan · 2017-07-14T20:55:34Z

@funny-falcon this trick doesn't help :-(

akzhan · 2017-07-14T20:56:35Z

@Sija @funny-falcon I'll drop constructor(BigRational) due to it's unpredictable usage behavior by current Crystal compiler.

Anyway we should prefer to_big_f.

Sija · 2017-07-14T21:00:34Z

shouldn't constructor(BigRational) be placed in big/big_rational.cr instead?

akzhan · 2017-07-14T21:01:59Z

Ok, will try :)

RX14 · 2017-07-14T17:34:11Z

src/big/big_float.cr

+  end
+
+  def /(other : UInt8 | UInt16 | UInt32)
+    raise DivisionByZero.new if other == 0


How about just self / LibGMP::ULong.new(other)

It's ok too.

funny-falcon · 2017-07-15T12:09:27Z

Excuse me, I was wrong in previous comment.

akzhan · 2017-08-28T10:48:16Z

@RX14 looks like ready to approve

funny-falcon · 2017-08-28T12:27:44Z

@funny-falcon Agrrhh, I can't reproduce bug that found by #4653 (comment)

@akzhan here is how it could be reproduced on master (and my branch) quite easy:
here is example when exception should be thrown, but it were not:

yura@falcon-work:~/Projects/crystal$ git diff           
diff --git a/src/big/big_float.cr b/src/big/big_float.cr
index d747e0c73..361fbec7c 100644                       
--- a/src/big/big_float.cr                              
+++ b/src/big/big_float.cr                              
@@ -17,6 +17,12 @@ struct BigFloat < Float              
     LibGMP.mpf_init_set_str(out @mpf, str, 10)         
   end                                                  
                                                        
+  def initialize(rat : BigRational)                    
+    raise "Rat"                                        
+    LibGMP.mpf_init(out @mpf)               
+    LibGMP.mpf_set_q(self, num)             
+  end                                                  
+                                                       
   def initialize(num : Number)                         
     LibGMP.mpf_init_set_d(out @mpf, num.to_f64)        
   end                                                  

$ bin/crystal eval 'require "big"; p BigFloat.new(BigRational.new(1,1))'
Using compiled compiler at `.build/crystal'                                                                
1_big_f

akzhan · 2017-08-28T13:00:56Z

@RX14 thanks to @funny-falcon - now broken overload issue has been extracted to #4897

RX14 · 2017-08-28T23:27:35Z

src/big/big_float.cr

@@ -18,7 +18,37 @@ struct BigFloat < Float
  end

  def initialize(num : Number)
-    LibGMP.mpf_init_set_d(out @mpf, num.to_f64)
+    case num


Does this new arrangement where there is no Number overload trigger the bug? If no we should use overloads here.

Yes, it is workaround.
Here was comment "XXX fix me when overload will work", but Akzhan removed it.

Now will be extracted as issue and link added

But does this specific code still break the specs. If it does not (and I don't believe it does, as you removed the Int overload) the workaround should be removed.

@RX14 this code once breaks the specs in some ways, use case has been extracted to #4897.

We must do this way until #4897 be fixed.

Just tested, extracted overloads works ok.

But it doesn't OK because I don't know why it sometimes didn't work - as exposed by #4897.

Compiler has unpredictable behavior to choose which overload will be chosen :-/

@RX14 - Moreover, added overloads break specs at all. Again.

1) BigFloat new new(BigInt) Failure/Error: bigfloat_on_bigint_value.should eq(bigfloat_of_integer_value) Expected: 123456789012345678901_big_f got: 123456789012345667584_big_f # spec/std/big/big_float_spec.cr:18 2) BigFloat new new(BigRational) Failure/Error: bigfloat_on_bigrational_value.should eq(BigFloat.new(1) / BigFloat.new(3)) Expected: 0.333333333333333333333_big_f got: 0.33333333333333331483_big_f

I hate Crystal compiler overloads for now. They have unpredictable behavior.

The bug is very clearly related to using Number or Int overloads (both of which are superclasses of BigInt). Simply avoid such generic overloads. Your case statement didn't have them, you don't need them.

There is no need to be "scared" of a bug, there is almost 0 chance that the bug will depend on anything but the list of overloads and their order. The bug seems fairly predictable to me.

I have initialize(num : Number) anyway

akzhan · 2017-08-29T13:49:24Z

@RX14 What can I do with these compiler bugs after returning of overloads? I want to rollback proposed overloads due to its erroneous.

by the way - bin/crystal spec/std/big/big_float_spec.cr works ok (failed only std spec)

Previous solution works fine in any conditions. No Crystal Voodoo overloads magic.

funny-falcon · 2017-08-29T14:28:44Z

@RX14 Travis-CI failed with constructor overloads. Until compiler fixed, code should be reverted to 'case' overload.

RX14 · 2017-08-29T14:48:00Z

src/big/big_float.cr

+      LibGMP.mpf_set_z(self, num.to_big_i)
+    end
+  end
+
  def initialize(num : Number)


As I suspected, removing this type restriction fixes the issue.

Yes, def initialize(num) works.
But it is ridiculous.
Should this method accept anything with #to_f64 method, or it should accept only Number?
If latter, then this restriction could not be removed.

I'd rather merge this with the overloads working and not have to change it later than merge it with the ugly case + comment in initialize. We can reproduce the failure so we can investigate it in parallel.

Just fix overloads by forward declaration of types as @asterite points to.

RX14

Finally!

akzhan · 2017-08-31T21:24:32Z

Good time to merge :-)

RX14 · 2017-08-31T21:39:04Z

Need another review on this before it's merged.

Prepare hash infrastructor to future change of hashing algrorithm to protect against Hash DoS. Class|Struct should define method `def hash(hasher)` and call `hasher << @ivar` inside. As an option, for speed, and for backward compatibility, `def hash` still could be implemented. It will be used for Hash of matched type. `Thread#hash` and `Signal#hash` is implemented as unseeded cause they are used before `StdHasher @@seed` is initialized. Hash::Hasher is default hasher that uses `hash(hasher)` and it is used as default seeded hasher. Also, number normalization for hashing introduced, ie rule 'equality forces hash equality' is forced (`a == b` => `a.hash == b.hash`). Normalization idea is borrowed from Python implementation. It fixes several issues with BigInt and BigFloat on 32bit platform, but not all issues. Fixes crystal-lang#4578 Fixes crystal-lang#3932 Prerequisite for crystal-lang#4557 Replaces crystal-lang#4581 Correlates with crystal-lang#4653

akzhan changed the title ~~Math.frexp(value : BigFloat) has native implementation.~~ Math.frexp(value : BigFloat | BigInt) has native implementation. Jul 2, 2017

akzhan mentioned this pull request Jul 6, 2017

change computation of hash value. #4675

Closed

funny-falcon reviewed Jul 6, 2017

View reviewed changes

akzhan force-pushed the bigfloat-frexp branch from 7763234 to 267463a Compare July 6, 2017 17:53

akzhan changed the title ~~Math.frexp(value : BigFloat | BigInt) has native implementation.~~ Additions to Big arithmetics Jul 7, 2017

makenowjust added a commit to makenowjust/crystal that referenced this pull request Jul 13, 2017

Fix to work crystal-lang#4663

77fff34

The ASTNode of `1 <= 2 <= 3` is `to_s`-ed to `(1 <= 2) <= 3`. Right side parenthesis is not required and it causes crystal-lang#4653 error.

makenowjust mentioned this pull request Jul 13, 2017

Fix chained comparisons to_s to work #4663 #4708

Merged

akzhan force-pushed the bigfloat-frexp branch from 6dca3b5 to 44d90e7 Compare July 14, 2017 20:24

Sija reviewed Jul 14, 2017

View reviewed changes

RX14 requested changes Jul 14, 2017

View reviewed changes

akzhan changed the title ~~Additions to Big arithmetics~~ [WIP] Additions to Big arithmetics Jul 16, 2017

akzhan added 2 commits August 28, 2017 16:02

Merge branch 'master' into bigfloat-frexp

0a4e266

more verbose naming for used variables.

3c0b038

akzhan mentioned this pull request Aug 28, 2017

[WIP] Wrong overload called instead of BigFloat.initialize(BigRational). #4893

Closed

RX14 reviewed Aug 28, 2017

View reviewed changes

akzhan added 3 commits August 29, 2017 12:46

just explanation of workaround, refs crystal-lang#4897

572ba07

For now overloads work (i don't know why).

40a06b2

dedup

651814f

RX14 reviewed Aug 29, 2017

View reviewed changes

akzhan added 2 commits August 31, 2017 00:34

Merge remote-tracking branch 'upstream/master' into bigfloat-frexp

833399f

Forward declarations of Big Arithmentics types.

a2bab2b

RX14 approved these changes Aug 30, 2017

View reviewed changes

asterite approved these changes Sep 1, 2017

View reviewed changes

RX14 added kind:feature topic:stdlib labels Sep 1, 2017

RX14 added this to the Next milestone Sep 1, 2017

RX14 merged commit d1960a5 into crystal-lang:master Sep 1, 2017

akzhan deleted the bigfloat-frexp branch September 1, 2017 14:17

akzhan mentioned this pull request Sep 3, 2017

Fixes to bigint 32bit #4922

Merged

Additions to Big arithmetics #4653

Additions to Big arithmetics #4653

Conversation

akzhan commented Jul 1, 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

funny-falcon commented Jul 7, 2017

funny-falcon commented Jul 7, 2017

akzhan commented Jul 7, 2017

akzhan commented Jul 7, 2017

funny-falcon commented Jul 7, 2017

akzhan commented Jul 7, 2017 • edited Loading

funny-falcon commented Jul 7, 2017

funny-falcon commented Jul 7, 2017

funny-falcon commented Jul 7, 2017

akzhan commented Jul 7, 2017

akzhan commented Jul 7, 2017

RX14 commented Jul 14, 2017

akzhan commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akzhan commented Jul 14, 2017

akzhan commented Jul 14, 2017 • edited Loading

Sija commented Jul 14, 2017

akzhan commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funny-falcon commented Jul 15, 2017

akzhan commented Aug 28, 2017

funny-falcon commented Aug 28, 2017 • edited Loading

akzhan commented 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

RX14 Aug 29, 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 29, 2017 • edited Loading

Choose a reason for hiding this comment

akzhan Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

RX14 Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akzhan commented Aug 29, 2017 • edited Loading

funny-falcon commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 left a comment

Choose a reason for hiding this comment

akzhan commented Aug 31, 2017

RX14 commented Aug 31, 2017

akzhan commented Jul 1, 2017 •

edited

Loading

akzhan commented Jul 7, 2017 •

edited

Loading

akzhan commented Jul 14, 2017 •

edited

Loading

funny-falcon commented Aug 28, 2017 •

edited

Loading

akzhan commented Aug 28, 2017 •

edited

Loading

RX14 Aug 29, 2017 •

edited

Loading

akzhan Aug 29, 2017 •

edited

Loading

akzhan Aug 29, 2017 •

edited

Loading

RX14 Aug 29, 2017 •

edited

Loading

akzhan commented Aug 29, 2017 •

edited

Loading

RX14 Aug 29, 2017 •

edited

Loading