Complex enhancements #5440

Sija · 2017-12-22T18:38:38Z

~~Made Complex descendant of Number (supersedes Let Complex inherit from Number #5436)~~
Added Complex#to_* family of conversion methods
Tweaked docs a bit here & there

RX14 · 2017-12-22T19:35:19Z

I agree with @larubujo. Complex isn't even comparable to itself. It makes little sense for it to descend from Number given Number's current interface.

Sija · 2017-12-22T19:45:01Z

Complex isn't even comparable to itself.

Hmm, I beg to differ:

crystal/spec/std/complex_spec.cr

Lines 13 to 37 in e04565f

    
           describe "==" do 
        
             it "complex == complex" do 
        
               a = Complex.new(1.5, 2) 
        
               b = Complex.new(1.5, 2) 
        
               c = Complex.new(2.25, 3) 
        
               (a == b).should be_true 
        
               (a == c).should be_false 
        
             end 
        
             it "complex == number" do 
        
               a = Complex.new(5.3, 0) 
        
               b = 5.3 
        
               c = 4.2 
        
               (a == b).should be_true 
        
               (a == c).should be_false 
        
             end 
        
             it "number == complex" do 
        
               a = -1.75 
        
               b = Complex.new(-1.75, 0) 
        
               c = Complex.new(7.2, 0) 
        
               (a == b).should be_true 
        
               (a == c).should be_false 
        
             end 
        
           end

RX14 · 2017-12-22T20:01:21Z

@Sija

The Comparable mixin is used by classes whose objects may be ordered.

Complex is not ordered.

Sija · 2017-12-22T20:34:06Z

@RX14 true, yet it's still a Number. All other numeric classes (BigInt, BigFloat, BigRational, BigDecimal) in stdlib derive from Number. IIRC Ruby does the same.

larubujo · 2017-12-22T21:05:49Z

ruby undefines methods for complex. not such thing in crystal. what do you win with complex < number? what can you do now that you cant before?

RX14 · 2017-12-22T22:37:22Z

@Sija yes complexes are numbers. That doesn't mean it has to inherit Number per se.

If it doesn't fit in the interface for Number, it's not a Number regardless of if it's a number. Yes that points to our number hierarchy being inconsistent with pure mathematics but i'm not sure how important that is in the real world, or if it's worth adding many new interfaces for it like I think Rust has done.

straight-shoota · 2017-12-22T22:37:51Z

@Sija A complex number is a number in real world (no pun intended). But it does not adequatly qualify as an instance of Number which expects operations that are simply not applicable to complex numbers.
Now, this could mean we need to think about if the features of Number might need a change. But at the current state, it just makes no sense to let Complex inherit from Number.

Sija · 2017-12-23T03:14:36Z

Fair points, thanks! I understand that, OTOH I see Complex extending Number with arithmetic operations and equality operator, making me ask myself what's the actual difference between them API-wise—spaceship <=> operator?

Perhaps every Number class descendant should inherit Comparable as opt-in instead? Are they any other incompatibilities between their APIs?

What about two other points listed?

straight-shoota · 2017-12-23T12:43:41Z

src/complex.cr

+  # raises otherwise.
+  def to_f64
+    unless @imag.zero?
+      raise Exception.new "The imaginary part should be exactly zero"


The error message should simply describe what's wrong, not how it should be, for example: Complex number with non-zero imaginary part can't be converted to real number.

Changed, thanks!

ghost · 2017-12-23T12:56:02Z

Going crazy with algebra is probably useless but having Number without <=> and having Conparable as a separate Module makes sense to me.
Finite fields have no ordering as well (F_2={0,1}) and are actually used for coding/ decoding information.
Ruby has Comparable as a module as well although they include it in number directly.
Having conparable as a separate include in every number class that supports it makes sense to me.

In practise for me a number is an element of a field and a field has no inherent ordering.

Someone else might argue though that a pointer to a variable is a number and arguing from this perspective a number is not necessarily a field (you can’t multiply pointers) but you certainly can compare and order their addresses.

RX14

I seriously think this is a badly thought out PR trying to shoehorn a Complex into being something it's not: a single-dimensional dumber.

RX14 · 2017-12-23T16:06:40Z

src/complex.cr

+  # raises otherwise.
+  def to_f64
+    unless @imag.zero?
+      raise Exception.new "Complex number with non-zero imaginary part can't be converted to real number"


But what's the point of these methods then? Why not just provide an assert_real! and an assert_imaginary! method and let people use c.real? What's the usecase of a number conversion method that raises all the time?

But what's the point of these methods then? Why not just provide an assert_real! and an assert_imaginary! method and let people use c.real? What's the usecase of a number conversion method that raises all the time?

Somehow that's what Ruby does, so I guess all of your questions should be directly answered by digging into the Ruby land, their decision making and API designing process.

I seriously think this is a badly thought out PR trying to shoehorn a Complex into being something it's not: a single-dimensional dumber.

And what's the point of of your criticism? I don't see any constructive arguments, only your personal opinions which you hold dearly to the point of putting yourself on airs.

If it doesn't fit in the interface for Number, it's not a Number regardless of if it's a number. Yes that points to our number hierarchy being inconsistent with pure mathematics but i'm not sure how important that is in the real world, or if it's worth adding many new interfaces for it like I think Rust has done.

AFAIK the only thing from Number API it doesn't fit is <=> operator which IMO shouldn't be included there from default.

Sija · 2018-08-17T03:43:53Z

Could this PR be re-reviewed?

Sija · 2018-08-31T17:38:35Z

I reverted the change making Complex a Number descendant.
Could this be re-reviewed?

Sija · 2018-11-16T15:12:00Z

🏓

src/complex.cr

This reverts commit 6462a40.

Sija · 2018-11-22T17:30:10Z

No idea why Travis failed...

RX14 · 2018-11-27T18:27:46Z

There's an incorrect sign on the imaginary part, no idea how that happened given the specs are run in the exact same docker container. Nothing could be different between circle and travis!

Sija · 2018-11-28T17:56:04Z

[...] no idea how that happened given the specs are run in the exact same docker container. Nothing could be different between circle and travis!

I broke CI :( what did I do wrong...? 💭 🐛🔨

src/complex.cr

Sija force-pushed the complex-enhancements branch from 1a794ec to 0b9196e Compare December 22, 2017 18:42

straight-shoota reviewed Dec 23, 2017

View reviewed changes

Sija force-pushed the complex-enhancements branch from 0b9196e to e5c58b7 Compare December 23, 2017 13:53

RX14 requested changes Dec 23, 2017

View reviewed changes

Sija force-pushed the complex-enhancements branch from cb740c4 to 79e1a82 Compare August 31, 2018 17:40

RX14 approved these changes Nov 22, 2018

View reviewed changes

RX14 reviewed Nov 22, 2018

View reviewed changes

src/complex.cr Show resolved Hide resolved

RX14 requested a review from bcardiff November 22, 2018 16:01

Sija added 4 commits November 22, 2018 17:25

Make Complex descendant of Number

0204eb8

Added #to_* conversion methods to Complex

b70d58c

Complex documentation tweaks

a957528

Revert "Make Complex descendant of Number"

9d869eb

This reverts commit 6462a40.

Sija force-pushed the complex-enhancements branch 2 times, most recently from 9c6a363 to 9d869eb Compare November 22, 2018 16:42

bcardiff requested changes Nov 30, 2018

View reviewed changes

src/complex.cr Outdated Show resolved Hide resolved

Complex#to_i should delegate to #to_i32

16a1572

bcardiff approved these changes Nov 30, 2018

View reviewed changes

Add Complex#+

500e369

RX14 approved these changes Dec 1, 2018

View reviewed changes

RX14 added this to the 0.27.1 milestone Dec 1, 2018

RX14 added kind:feature topic:stdlib labels Dec 1, 2018

RX14 merged commit e00cf8c into crystal-lang:master Dec 1, 2018

bcardiff added topic:stdlib:numeric and removed topic:stdlib labels Jan 28, 2019

jhass mentioned this pull request Sep 1, 2020

+(-1 - 1.i) return (1 +1.i) #9709

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complex enhancements #5440

Complex enhancements #5440

Sija commented Dec 22, 2017 •

edited

RX14 commented Dec 22, 2017

Sija commented Dec 22, 2017

RX14 commented Dec 22, 2017 •

edited

Sija commented Dec 22, 2017

larubujo commented Dec 22, 2017

RX14 commented Dec 22, 2017

straight-shoota commented Dec 22, 2017

Sija commented Dec 23, 2017 •

edited

straight-shoota Dec 23, 2017

Sija Dec 23, 2017

ghost commented Dec 23, 2017

RX14 left a comment •

edited

RX14 Dec 23, 2017

Sija Dec 23, 2017

Sija commented Aug 17, 2018

Sija commented Aug 31, 2018

Sija commented Nov 16, 2018

Sija commented Nov 22, 2018

RX14 commented Nov 27, 2018

Sija commented Nov 28, 2018

Complex enhancements #5440

Complex enhancements #5440

Conversation

Sija commented Dec 22, 2017 • edited

RX14 commented Dec 22, 2017

Sija commented Dec 22, 2017

RX14 commented Dec 22, 2017 • edited

Sija commented Dec 22, 2017

larubujo commented Dec 22, 2017

RX14 commented Dec 22, 2017

straight-shoota commented Dec 22, 2017

Sija commented Dec 23, 2017 • edited

straight-shoota Dec 23, 2017

Choose a reason for hiding this comment

Sija Dec 23, 2017

Choose a reason for hiding this comment

ghost commented Dec 23, 2017

RX14 left a comment • edited

Choose a reason for hiding this comment

RX14 Dec 23, 2017

Choose a reason for hiding this comment

Sija Dec 23, 2017

Choose a reason for hiding this comment

Sija commented Aug 17, 2018

Sija commented Aug 31, 2018

Sija commented Nov 16, 2018

Sija commented Nov 22, 2018

RX14 commented Nov 27, 2018

Sija commented Nov 28, 2018

Sija commented Dec 22, 2017 •

edited

RX14 commented Dec 22, 2017 •

edited

Sija commented Dec 23, 2017 •

edited

RX14 left a comment •

edited