-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Complex enhancements #5440
Complex enhancements #5440
Conversation
1a794ec
to
0b9196e
Compare
I agree with @larubujo. Complex isn't even comparable to itself. It makes little sense for it to descend from |
Hmm, I beg to differ: crystal/spec/std/complex_spec.cr Lines 13 to 37 in e04565f
|
Complex is not ordered. |
@RX14 true, yet it's still a |
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? |
@Sija yes complexes are numbers. That doesn't mean it has to inherit If it doesn't fit in the interface for |
@Sija A complex number is a number in real world (no pun intended). But it does not adequatly qualify as an instance of |
Fair points, thanks! I understand that, OTOH I see Perhaps every What about two other points listed? |
src/complex.cr
Outdated
# raises otherwise. | ||
def to_f64 | ||
unless @imag.zero? | ||
raise Exception.new "The imaginary part should be exactly zero" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed, thanks!
Going crazy with algebra is probably useless but having Number without <=> and having Conparable as a separate Module 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. |
0b9196e
to
e5c58b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Could this PR be re-reviewed? |
I reverted the change making |
cb740c4
to
79e1a82
Compare
🏓 |
9c6a363
to
9d869eb
Compare
No idea why Travis failed... |
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! |
I broke CI :( what did I do wrong...? 💭 🐛🔨 |
MadeComplex
descendant ofNumber
(supersedes Let Complex inherit from Number #5436)Complex#to_*
family of conversion methods