[RFC] Add routines for degrees/radians conversions

[PureFox48]

Although the Num class has good support for trig methods, one thing it lacks is methods to convert between degrees and radians.

Whilst these are easy to code, they're also easy to get the wrong way around. I therefore propose adding the following to the Num class:

toRadians { this * Num.pi / 180 }

toDegrees { this * 180 / Num.pi }

Possibly we could drop to and just use radians and degrees instead?

Alternatively, we could code these methods in C and hard-code pi as 3.14159265358979323846.

Thoughts anyone?

[ghost]

Having them in C would be better, it adds speed to convenience.

[PureFox48]

Yeah, I agree C would be better. Which do you prefer with to or without?

[ghost]

I think the to prefix is better, otherwise the names would be ambiguous: "does it convert from radians to degrees or vice-versa ?"
Now we could debate about 'to' vs 'from' but that would be pointless IMHO

[PureFox48]

Good point, having the prefix is certainly clearer.

I'll wait and see if there are any further comments before doing a PR.

[mhermier]

I don't think it is a good idea in the main library, but anyways, if we include that, I think it is better to provide the original unit in the naming convention. `radiansToDegrees` `tausToDegrees`...

[PureFox48]

I don't think it is a good idea in the main library

Why's that? The trig methods are in the main library so you hardly want to be importing am external library just to do these conversions. Even in C it's only adding a few lines to core.

I think it is better to provide the original unit in the naming convention. radiansToDegrees tausToDegrees

The trouble with those names is that they're very long winded. I think people would be tempted to take their chances with the formulae rather than writing those out.

Python has math.radians(_) and math.degrees(_) & Lua has math.rad(_) and math.deg(_) . So I suppose we could use static rather than instance methods. However, Wren is an OO language and I think we should stick to instance methods where feasible.

[PureFox48]

Just checked Gravity and they have radians and degrees as instance methods of the Float class.

[mhermier]

I'm not really happy with the extra symbol pollution (even the one inside Num). Num as a bunch of unique symbols that are not very good for cache locality accessing symbols (though it has to be proven).
It is a complex topic, but I tend to prefer to have extra computation external to a class unless necessary these days (what ever the language).
So I think, unless there is a huge need for it, I would implement it in a separated module (and also migrate trigonometry and some utility stuff there in the process). It does not follow OO design, but we can't cargo all math classic formulas and constants in existence in the name of it...

[PureFox48]

Well if you move the trig methods out of Num and into a separate module (Math say) then it will break every program that uses those methods:

1. Unless Math is itself part of core, then it will need to be explicitly imported.

2. As there will no longer be an object (i.e. a number), the methods will need to be static and so the invocations themselves will all need to be changed.

I'd have thought the chances of that happening are next to nil as the trig methods are used a lot in games programming which is an important use case for an embedded scripting language such as Wren.

Degrees/radians conversion methods are not essential - it's just a matter of convenience so you don't have to remember the formula each time a conversion is needed and risk getting it wrong. If they aren't in the Num class (or at least somewhere else in core), then that convenience goes out of the window as you have to import their containing module. I have these methods in my own Math module but, unless I'm importing it for some other reason (it's 700+ lines), I don't use them - I just use the formulae directly.

[mhermier]

If API stability is a question while we are in 0.4.0 then, we somehow failed. I mean, it will not be a pleasant change, but not all usage of wren would require to perform trigonometric math, even in video games. The important thing is to have them available, in some form. The biggest problem with these methods, is that they have mostly unique signatures, which would grow the methods arrays unreasonably, which would cause cache misses for something of relatively *limited* use. By moving them to a module, and thus delaying their declarations, we limit that behaviour, and give priori to more hot methods signatures.

[ruby0x1]

I haven't found this to be an issue, and it's unlikely we'll remove the convenient API we've had all along.

[mhermier]

The followinf example is a little bit made up, but it expose the problem at long term:

class Benchmark {
  static call(benchmark) { call("", benchmark) }

  static call(name, benchmark) {
    var start = System.clock
    benchmark.call()
    var end = System.clock
    System.print("[%(name)] elapsed: %(end - start)")
  }
}

var numTests = 100000000

var func = Fn.new {|self| self }

class Obj {
  static delinquant_method_name_00 {}
  static delinquant_method_name_01 {}
  static delinquant_method_name_02 {}
  static delinquant_method_name_03 {}
  static delinquant_method_name_04 {}
  static delinquant_method_name_05 {}
  static delinquant_method_name_06 {}
  static delinquant_method_name_07 {}
  static delinquant_method_name_08 {}
  static delinquant_method_name_09 {}
  static delinquant_method_name_10 {}
  static delinquant_method_name_11 {}
  static delinquant_method_name_12 {}
  static delinquant_method_name_13 {}
  static delinquant_method_name_14 {}
  static delinquant_method_name_15 {}
  static delinquant_method_name_16 {}
  static delinquant_method_name_17 {}
  static delinquant_method_name_18 {}
  static delinquant_method_name_19 {}
  static delinquant_method_name_20 {}
  static delinquant_method_name_21 {}
  static delinquant_method_name_22 {}
  static delinquant_method_name_23 {}
  static delinquant_method_name_24 {}
  static delinquant_method_name_25 {}
  static delinquant_method_name_26 {}
  static delinquant_method_name_27 {}
  static delinquant_method_name_28 {}
  static delinquant_method_name_29 {}
  static delinquant_method_name_30 {}
  static delinquant_method_name_31 {}
  static delinquant_method_name_32 {}
  static delinquant_method_name_33 {}
  static delinquant_method_name_34 {}
  static delinquant_method_name_35 {}
  static delinquant_method_name_36 {}
  static delinquant_method_name_37 {}
  static delinquant_method_name_38 {}
  static delinquant_method_name_39 {}
  static delinquant_method_name_40 {}
  static delinquant_method_name_41 {}
  static delinquant_method_name_42 {}
  static delinquant_method_name_43 {}
  static delinquant_method_name_44 {}
  static delinquant_method_name_45 {}
  static delinquant_method_name_46 {}
  static delinquant_method_name_47 {}
  static delinquant_method_name_48 {}
  static delinquant_method_name_49 {}
  static delinquant_method_name_50 {}
  static delinquant_method_name_51 {}
  static delinquant_method_name_52 {}
  static delinquant_method_name_53 {}
  static delinquant_method_name_54 {}
  static delinquant_method_name_55 {}
  static delinquant_method_name_56 {}
  static delinquant_method_name_57 {}
  static delinquant_method_name_58 {}
  static delinquant_method_name_59 {}

  static my_uniquely_named_static_method(self) { self }
  static call(self) { self }

  delinquant_method_name_00 {}
  delinquant_method_name_01 {}
  delinquant_method_name_02 {}
  delinquant_method_name_03 {}
  delinquant_method_name_04 {}
  delinquant_method_name_05 {}
  delinquant_method_name_06 {}
  delinquant_method_name_07 {}
  delinquant_method_name_08 {}
  delinquant_method_name_09 {}
  delinquant_method_name_10 {}
  delinquant_method_name_11 {}
  delinquant_method_name_12 {}
  delinquant_method_name_13 {}
  delinquant_method_name_14 {}
  delinquant_method_name_15 {}
  delinquant_method_name_16 {}
  delinquant_method_name_17 {}
  delinquant_method_name_18 {}
  delinquant_method_name_19 {}
  delinquant_method_name_20 {}
  delinquant_method_name_21 {}
  delinquant_method_name_22 {}
  delinquant_method_name_23 {}
  delinquant_method_name_24 {}
  delinquant_method_name_25 {}
  delinquant_method_name_26 {}
  delinquant_method_name_27 {}
  delinquant_method_name_28 {}
  delinquant_method_name_29 {}
  delinquant_method_name_30 {}
  delinquant_method_name_31 {}
  delinquant_method_name_32 {}
  delinquant_method_name_33 {}
  delinquant_method_name_34 {}
  delinquant_method_name_35 {}
  delinquant_method_name_36 {}
  delinquant_method_name_37 {}
  delinquant_method_name_38 {}
  delinquant_method_name_39 {}
  delinquant_method_name_40 {}
  delinquant_method_name_41 {}
  delinquant_method_name_42 {}
  delinquant_method_name_43 {}
  delinquant_method_name_44 {}
  delinquant_method_name_45 {}
  delinquant_method_name_46 {}
  delinquant_method_name_47 {}
  delinquant_method_name_48 {}
  delinquant_method_name_49 {}
  delinquant_method_name_50 {}
  delinquant_method_name_51 {}
  delinquant_method_name_52 {}
  delinquant_method_name_53 {}
  delinquant_method_name_54 {}
  delinquant_method_name_55 {}
  delinquant_method_name_56 {}
  delinquant_method_name_57 {}
  delinquant_method_name_58 {}
  delinquant_method_name_59 {}

  construct new() { }
  my_uniquely_named_method() { this }
  call() { this }
}

var obj = Obj.new()

Benchmark.call("function") {
  for (i in 0..numTests) func.call(obj)
}

Benchmark.call("static unique") {
  for (i in 0..numTests) Obj.my_uniquely_named_static_method(obj)
}

Benchmark.call("static") {
  for (i in 0..numTests) Obj.call(obj)
}

Benchmark.call("method unique") {
  for (i in 0..numTests) obj.my_uniquely_named_method()
}

Benchmark.call("method") {
  for (i in 0..numTests) obj.call()
}

For me the effect start to be visible after > 50 unique symbols. And functional style become more viable.

[function] elapsed: 8.992618
[static unique] elapsed: 9.0859
[static] elapsed: 8.560656
[method unique] elapsed: 8.463672
[method] elapsed: 8.659075

[PureFox48]

Can I start by making it clear that I'm not against breaking changes pre v1.0 if there's a good reason for them - and for a huge change such as the one you're proposing there would have to be a very good reason.

Wren has, after all, been in development for about nine years and during that time we appear to have broken very little in the language/core library - so it's not unreasonable for people to have some expectation of stability here.

But the benefits of the change are vague at best. When I run the benchmarks you've just posted several times, the figures are all over the place and, as we're talking about caching, are no doubt machine dependent.

Sure, one can argue that the trig methods should be static rather than instance and I presume you made that argument when they were introduced. It's also true that the Num class has quite a lot of symbols and so you'll be relieved to hear that, after this one, I don't intend to suggest adding any more :)

[datatypevoid]

Fear of breaking things is a terrible excuse not to innovate or improve a project, especially pre 1.0

[PureFox48]

I've decided to close this issue as I see no prospect of agreement on a way forward.

Also, if there's a valid concern that Num is acquiring too many methods, then I don't want to add to that.

Conversion methods are in any case a 'nice to have' rather than an 'essential' feature.

Thanks for the discussion everyone.

[CrazyInfin8]

Even though you closed this, I'd like to suggest another option.

In HaxeFlixel, we have two static constants FlxAngle.TO_DEG and FlxAngle.TO_RAD which you multiply against the angle (assumed to be the opposite of either radian or degrees to get the other value). This way, we won't have to worry about the methods like Num.degree or Num.radian hiding complexity since these methods would be as complex as Num.pi or Num.tau. Instead, the user would have to intensionally multiply their values to convert, which isn't overly verbose.

Also if we would like to be verbose like: "to degrees" or "radians to degrees" we could also shorten the unit like so: Num.toDeg, Num.DegToRad, Num.Rad2Deg, etc.

[PureFox48]

It's a sensible way to proceed and one I'd personally be happy with. My preferred names would be Num.toDeg and Num.toRad as they're not too long and reasonably clear.

We would still be adding more methods to the Num class though they'd be static rather than instance methods and this might be an acceptable compromise to those on both sides of the argument.

Reopening for further comment.

[mhermier]

I don't like how Num is becoming a god object. In some aspect by sticking too much to OOP, it deafeat the point of OOP...

[PureFox48]

Well, @CrazyInfin8's idea gets away from OO to some extent by using a static property rather than an instance method.

But the reason why I decided to suggest built-in conversion methods in the first place is because I'm in the middle of designing a module for doing vector arithmetic (in the geometric rather than C++ sense) and have been finding it a pain having to convert degrees to radians and back again when testing stuff such as polar, cylindrical and spherical coordinates. Although I have these conversion methods in my math module I don't like my modules to have dependencies on other large modules just to import a bit of code and so I thought it would be nice if these were available as a built-in.

The alternative would be to build them into my Vector class somehow even it means not sticking to the DRY principle.

Incidentally, if you look through the existing PRs, there are quite a few which involve adding more stuff to Num including one from yourself to add a unary plus operator which I agree with! So, I'd prefer this proposal to be judged on its merits rather than rejected just because there are already a lot of methods in Num.

[mhermier]

The subject is complex and sometimes politic enter into play. But for me, these MUST be implemented as method:

• methods that access internal data for legitimate reasons, or that could not be implemented otherwise
• methods that needs overload mechanism
• operators that make sense

The rest is in the gray area, but I tend to want them as external functions by default. So it is not a judgement on whether you or me are right or wrong, and being supportive. Yes, the idea to have conversion operation from different angular unit is valid. No I don't think it should not belong there, like many of the other Num methods (like the trigonometry ones), and others like in Sequence.

Strict OOP has proven to not scale over the years, and it is clearly visible these days. We are making the same mistake with all theses utility methods in Num. To make things worse, our implementation of the language has a performance problem with an increasing number of unique method signatures.

Ultimately, it doesn't matter whether or not we add them to Num, since we can always move them to a module quite easily. But, adding everything in core is a trade-off that scale less and less, even considering our primary usage.

[PureFox48]

I don't disagree with a lot of what you've just said.

It's certainly arguable that the trig methods are naturally static and should have been placed in a separate class to Num from the outset. But, we are where we are, and in view of what @ruby0x1 said above (and with which I certainly agree on practical grounds) they're unlikely to be removed from Num.

Even if they were moved to say an optional Math module, I'm not sure many people would bother to exclude it from their applications anyway even if they couldn't see a use for it. I wonder how many exclude the optional Random and Meta modules we already have?

So the question we come back to is whether angle conversion methods are worth having or not because if they are there's no other convenient place to put them apart from the Num class. Making them static eases the pressure a bit on the large number of instance methods we already have and which may be added to by the existing PRs.

Unless, there are any violent objections to this, I think I'll do a PR on the basis of @CrazyInfin8's suggestion (it's a very simple change) so there's something concrete on the table.