crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.24k stars 1.61k forks source link

Complex: mathematical functions should belong to Math instead of Complex itself #4160

Open makenowjust opened 7 years ago

makenowjust commented 7 years ago

Currently we can calculate exp of Complex by its instance methods, and log of Complex too: like Complex.new(1, 2).exp and Complex.new(1, 2).log. But exp of Float64 or Float32, we need to use Math.exp. It means, this robs the ability to write a generic function for Complex and Float from us. I think

  1. mathematical functions for Complex should belong to Math module instead Complex, or
  2. should belong to Number as instance method instead of Math as module method,

in other words I want to organize the way to apply mathematical functions.

I don't prefer too many methods on a class, so I think the first idea is better than the second. How do you think?

(I have a patch to define trigonometric functions for Complex. I'll make pull request when this issue is solved.)

bcardiff commented 7 years ago

In 2 you meant Math.exp (class/module method) instead of Number#exp (instance method), right?

My preference is to stick as near as the usual math notation.

I find a bit problematic to keep factorial usual notation. I wouldn't like 5!, 5.!, 5.fact. I would go with Math.fact(5)

Having all in Math has the benefit of uniform between Numbers, Complex (and Quaternion (?) ). And the user could pick include Math and remove all the Math. from the source, so it will be more math like code.

In a code organization detail: I would define all those methods in by reopening Math module when you include Complex for example.

makenowjust commented 7 years ago

@bcardiff

In 2 you meant Math.exp (class/module method) instead of Number#exp (instance method), right?

Yes. (~I will edit my issue text.~ done.)

  • I prefer c.exp(2) rather than Math.exp(c, 2) (as with binary operators)

I think so too with "as with binary operator", but Math.exp is not binary operator.

makenowjust commented 7 years ago

Memo:

bcardiff commented 7 years ago

I think so too with "as with binary operator", but Math.exp is not binary operator.

I mean to include binary operators in this syntax if they are not implement with infix operators like +,/,**.

Definitely exp it is not a binary operator, 5.exp(2) is 5 e ^ 2. Yet, I was so used to read `e^that I read it as a single thing. Maybeexp` could be present in both Math and Number.

makenowjust commented 7 years ago

For overloading Math.exp with Complex, the definition def exp(value) (without restriction version) disturbs it. We may need to redefine this with restriction as Number like def exp(value : Number). It is breaking change, but I think this effect is little.

jhass commented 7 years ago

Eliminating Math could be a fun experiment. For the factorial case I wouldn't mind x.fact or even x.factorial, for the trigonometric functions I actually wouldn't mind Number.sin(x) (with overloads for Complex instances there).

ozra commented 7 years ago

I would not like to see trigonometrics, etc. on Number -.

I prefer opening Math and use the methods "functionally", so agree strongly with @bcardiff here.

Agree with @MakeNowJust - Math.max/min should be variadic.

RX14 commented 7 years ago

I think using Tuple#min and Tuple#max is better than Math.min/Math.max. I recall @asterite telling me to use the former in a PR review somewhere. If that's the case, why not just remove them on Math?

ozra commented 7 years ago

It it has to be one or the other, think definitely min, max is better off removed from Tuple in such a case. Tuples != number collections.

Sija commented 7 years ago

@ozra you don't only compare numbers either (see Comparable).

ozra commented 7 years ago

True, still I've never used min and max for anything but numbers. Of course that is kind of a limited study ;-)

miketheman commented 6 years ago

triage

This discussion started and stopped quite quickly. In the interest of driving it forward to some sort of consensus and conclusion, what could be the next steps here?

HertzDevil commented 3 years ago

Is this resolved by #9739?