search

brownplt / code.pyret.org

Website for serving Pyret to folks.
Other
24 stars 44 forks source link

`rotate` doesn't accept non-integer angles #215

Closed blerner closed 6 years ago

blerner commented 7 years ago

rotate canonicalizes its angle by using jsnums.remainder, which asserts that its arguments are both integers. This is silly and unnecessary, and limits the programs we can write.

ds26gte commented 7 years ago

Yeah, definitely a bug. Before I fix it, did you mean jsnums.remainder requiring both its arguments to be ints is itself silly? Local tradition and/or accustomed expectation seem to require this (e.g., Racket, and of course WeScheme). (I am fine with remainder not using this assertion -- just wanted to confirm what your "This" was referring to.)

(Fixing rotate by rewriting it to not use integer remainder is easy enough.)

blerner commented 7 years ago

I think remainder is a utility function, so it should have as few restrictions as plausible. Restricting num-modulo to be Int, Int -> Int is fine (I'm totally ok with that), but is orthogonal to whether the js-numbers library requires that restriction. Are there any reasons why remainder is enforcing its integer view on all of its clients? (And, are there other clients that are similarly impacted?)

ds26gte commented 7 years ago

No, I don't think there is any reason for this restriction except received convention. Also: a quick grep -r reveals only rotate in the codebase uses remainder.

ds26gte commented 7 years ago

Submitted a pyret-lang PR for non-int remainder, but it may still be good to additionally have canonicalize-angle do an integer remainder on the integer part of the angle, and then add back the fractional part for precision. E.g., a huge rational number of degrees that is non-integral may lose a lot of precision if we take the fixnum remainder against 360. (This is very much in the nature of fine-tuning that may not find actual use.)