Make AngularVelocity a derivative of Angle #173
Conversation
|
Hi, Thanks for the PR. I put a few minor comments in-line. Would you also consider updating the tests? I'm thinking space/AngleSpec.scala. I'm wondering if space/SpaceChecks.scala should also be updated to have a general "AngularVelocity = Angle/Time" property. -derek |
shadaj
force-pushed
the
angular-velocity-integrals
branch
2 times, most recently
from
c048917
to
8a77066
Compare
|
I've added unit tests in AngleSpec, AngularVelocitySpec, and SpaceChecks that verify that transformations between Angle and AngularVelocity work correctly. I can't see the in-line comments, however. Can you resubmit them to GitHub? |
|
Thanks for adding the tests, and the quick turn around. My comments were around using Degrees and DegreesPerSecond in AngularVelocity and Angle, respectively. We try to use SI units when possible, meaning Radians in this case. It looks like you changed AngularVelocity.scala to use Radians in the updated PR, but Angle.scala still uses DegreesPerSecond on line 39. Could you change it to use RadiansPerSecond? Other than that, this looks good. -derek |
shadaj
force-pushed
the
angular-velocity-integrals
branch
from
8a77066
to
10c5b98
Compare
|
Awesome! I updated Angle.scala to use RadiansPerSecond. |
| @@ -34,6 +35,10 @@ final class Angle private (val value: Double, val unit: AngleUnit) | |||
| def tan = math.tan(toRadians) | |||
| def asin = math.asin(toRadians) | |||
| def acos = math.acos(toRadians) | |||
|
|
|||
| protected def timeDerived: AngularVelocity = RadiansPerSecond(toRadians) | |||
There was a problem hiding this comment.
I wonder why on AngularVelocity the protected methods are [squants] protected but not in Angle
There was a problem hiding this comment.
This is a timeDerived value. We're not consistent on those.
The other values in this PR are timeIntegrated values, which we're pretty good at making protected[squants].
timeDerived is all over the place, though.
Good question. We don't seem to be consistent in that.
In Velocity, the timeDerived member is public (line 31).
In Information, the timeDerived member is protected (line 27).
In LuminousEnergy, the timeDerived member is protected (line 25).
etc.
|
Thanks for the PR, it looks very good to me |
| def dimension = AngularVelocity | ||
|
|
||
| def toRadiansPerSecond = to(RadiansPerSecond) | ||
| def toDegreesPerSecond = to(DegreesPerSecond) | ||
| def toGradsPerSecond = to(GradsPerSecond) | ||
| def toTurnsPerSecond = to(TurnsPerSecond) | ||
|
|
||
| protected[squants] def timeIntegrated: Angle = Degrees(toDegreesPerSecond) |
There was a problem hiding this comment.
We generally use SI units inside the library. Should this be changed to use radians?
| @@ -34,6 +35,10 @@ final class Angle private (val value: Double, val unit: AngleUnit) | |||
| def tan = math.tan(toRadians) | |||
| def asin = math.asin(toRadians) | |||
| def acos = math.acos(toRadians) | |||
|
|
|||
| protected def timeDerived: AngularVelocity = DegreesPerSecond(toDegrees) | |||
There was a problem hiding this comment.
Same comment about radians.
| @@ -34,6 +35,10 @@ final class Angle private (val value: Double, val unit: AngleUnit) | |||
| def tan = math.tan(toRadians) | |||
| def asin = math.asin(toRadians) | |||
| def acos = math.acos(toRadians) | |||
|
|
|||
| protected def timeDerived: AngularVelocity = RadiansPerSecond(toRadians) | |||
There was a problem hiding this comment.
This is a timeDerived value. We're not consistent on those.
The other values in this PR are timeIntegrated values, which we're pretty good at making protected[squants].
timeDerived is all over the place, though.
Good question. We don't seem to be consistent in that.
In Velocity, the timeDerived member is public (line 31).
In Information, the timeDerived member is protected (line 27).
In LuminousEnergy, the timeDerived member is protected (line 25).
etc.
This makes it possible to multiply
AngularVelocitybyTimeto getAngle, and divideAnglebyTimeto get anAngularVelocity.