Rotational dynamics units #227

PhilipAxelrod · 2017-04-30T17:39:19Z

Create units related to rotational dynamics such as Torque, MomentOfInertia, and AngularAcceleration.

cquiroz

Thanks a lot for your contribution. It seems this fills an important missing piece on squants

I added some comments inline

cquiroz · 2017-05-01T00:20:48Z

shared/src/main/scala/squants/Quantity.scala


+abstract class StrictlyPositiveQuantity[A <: StrictlyPositiveQuantity[A]](value: Double) extends Quantity[A] {self: A ⇒


This is interesting though I'd strongly prefer have a constructor returning an Option[StrictlyPositiveQuantity[A]] than throwing an exception here

@cquiroz this feature leads to many different complications that are outside of the scope of this PR, so I have decided to remove it. I have addressed all your comments, and I think this PR is now ready to be merged! Thank you!

cquiroz · 2017-05-01T00:21:37Z

shared/src/main/scala/squants/mass/MomentOfIntertia.scala

+/**
+  *
+  * @author paxelord
+  * @since 0.6


We are at version 1.2

I will make sure to change this

cquiroz · 2017-05-01T00:24:31Z

shared/src/main/scala/squants/mass/MomentOfIntertia.scala

+  lazy val kilogramMetersSquared = KilogramsMetersSquared(1)
+  lazy val poundSquareFeet = PoundsSquareFeet(1)
+
+  implicit class MassConversions[A](n: A)(implicit num: Numeric[A]) {


Can this be made a value class? maybe not with the implicit there

I don't understand, can you please clarify?

If it helps, I accidentally misnamed what should have been MomentOfInertiaConversions to MassConversions by mistake. I don't even know how that happened.

Just chiming in, @paxelord and I are on the same robotics team, where we're using Squants. I think this can still be made into a value class, if the implicit is moved to the methods. So MassConversions (or MomentofInertiaConversions I guess) would only take a val n, and the kilogramMetersSquared and poundSquareFeet would take the implicit Numeric.

cquiroz · 2017-05-01T00:49:07Z

shared/src/main/scala/squants/mass/MomentOfIntertia.scala

+}
+
+object KilogramsMetersSquared extends MomentOfInertiaUnit with PrimaryUnit with SiBaseUnit {
+  val symbol = Kilograms.symbol + "‧" + Meters.symbol + "²"


Are we using exponentials in unicode like this in other places?

In Acceleration and Area, unicode exponentials are used.

cquiroz · 2017-05-01T00:50:43Z

shared/src/main/scala/squants/motion/AngularAcceleration.scala

+}
+
+object RadiansPerSecondSquared extends AngularAccelerationUnit with PrimaryUnit with SiUnit{
+  override val symbol: String = "rad/s²"


Wouldn't you use Radians.symbol here to be consistent?

My bad, I'll change that!

cquiroz · 2017-05-01T00:54:07Z

shared/src/main/scala/squants/motion/AngularAcceleration.scala

+}
+
+object DegreesPerSecondSquared extends AngularAccelerationUnit {
+  val symbol = "°/s²"


Same as above

cquiroz · 2017-05-01T00:55:53Z

shared/src/main/scala/squants/mass/MomentOfIntertia.scala

+}
+
+object MomentOfInertia extends Dimension[MomentOfInertia] {
+  private[mass] def apply[A](n: A, unit: MomentOfInertiaUnit)(implicit num: Numeric[A]) = new MomentOfInertia(num.toDouble(n), unit)


Could the constructors here return an Option to account for negative values?

I'm not 100% sure that's a good idea. People might be confused why they are getting a None.get Exception, whereas as throwing an IlleagalArgumentException would be more descriptive.

Throwing exceptions isn't very functional. If Option isn't suitable, we can look at Either which has a place to place an error data structure.

I will try experimenting with Option and Either, and try to figure out what is most natural

On a side note, I am starting to second guess whether this class should even exist. Maybe Squants should only provide compile time safety of units, and not tell people how to represent quantities, even if they are doing unusual things like having negative Mass or MomentOfInertia.

This might hinder people trying to explain legitimate scientific phenomena: https://en.wikipedia.org/wiki/Negative_mass

One compromise would be to have the strictly positive feature optional, enabled by default. Someone who needs to use negative mass would have to explicitly disable the strictly positive feature. I'm not completely sure how that would work though.

cquiroz · 2017-05-01T00:56:32Z

shared/src/main/scala/squants/motion/AngularAcceleration.scala

+  override protected[squants] def time: Time = Seconds(1)
+}
+
+


I will make sure to change this to just 1 line.

cquiroz · 2017-05-01T00:57:15Z

shared/src/main/scala/squants/motion/AngularVelocity.scala

+    * @param radius the distance from the center of rotation
+    * @return linear velocity with given angular velocity and radius
+    */
+  def onRadius(radius: Length): Velocity = {


You don't need the { parentheses here

cquiroz · 2017-05-01T01:00:20Z

shared/src/main/scala/squants/motion/Torque.scala

+}
+
+object NewtonMeters extends TorqueUnit with PrimaryUnit with  SiBaseUnit {
+  val symbol = Newtons.symbol + "‧" + Meters.symbol


Is this a times symbol? Are we using the same on other units?

This is meant to be the times symbol. I looked though squants, and I think this is the first time we have a times symbol in a unit. It might be clearer to replace the current bullet ‧ with a splat *

In a future PR, it might be useful to have have a Integral[Unit] and Derivative[Unit], similar to TimeDerivative and TimeIntegral, to allow for easy extendability of units. For example, Energy would extend Integral[Force] with Integral[Length]. Or even better, something along the lines of Integral[Integrand, RespectTo] But that's for a future PR.

cquiroz · 2017-05-07T19:41:04Z

shared/src/main/scala/squants/motion/AngularAcceleration.scala

  val conversionFactor = Degrees.conversionFactor
 }

-object GradiansPerSecondSquared extends AngularAccelerationUnit {
+object GradsPerSecondSquared extends AngularAccelerationUnit {


Why the change to Grads? Gradians seems clearer at the costs of just a few letters. In Spanish Grads can be easily confused by Degrees

I made it Grads instead of Gradians because that was the convention used in AngularVelocity. However, I agree with you about Gradians being clearer, and I will change it. Do you also want to change that in AngularVelocity?

Yes, I think it should be consistent with the units on Angle

https://github.com/typelevel/squants/blob/master/shared/src/main/scala/squants/space/Angle.scala#L66

cquiroz · 2017-05-07T19:41:54Z

shared/src/main/scala/squants/Quantity.scala

@@ -308,8 +308,3 @@ abstract class Quantity[A <: Quantity[A]] extends Serializable with Ordered[A] {
  def map(f: Double ⇒ Double): A = unit(f(value))
 }

-abstract class StrictlyPositiveQuantity[A <: StrictlyPositiveQuantity[A]](value: Double) extends Quantity[A] {self: A ⇒


Sorry about this. I think this can be made to work just making the constructors return Option or Either or even Try

The problem is that we run into complications such as these: Take the scenario KilogramsMetersSquared(1) - KilogramsMetersSquared(2) + KilogramsMetersSquared(2)
After the first subtraction, we get a negative moment, resulting in None. The addition afterwards would add a None with a KilogramsMetersSquared(2), getting None
However, mathematically, this should be evaluated to the equivalent of Some(KilogramsMetersSquared(1)). Implementing this kind of feature is certainly possible, but scenarios such as these make it complicated and outside the scope of this pr.

cquiroz · 2017-05-07T19:43:21Z

I'm ok with this PR as it is now but will wait for others to chime in, @derekmorr?

…uch as Torque, MomentOfInertia, and AngularAcceleration. In addition, add multiply methods to Angle and AngularVelocity

…sion from Energy to Torque, and simplify code

cquiroz · 2017-05-12T12:10:46Z

Can we proceed with this PR? It seems fine to me

derekmorr · 2017-05-12T12:43:28Z

My only objection was renaming GradsPerSecond to GradiansPerSecond since it changes existing API.

cquiroz · 2017-05-12T12:46:07Z

I see, I proposed that to make it consistent with Angle but right it would break the API. Could we deprecate the existing and add Gradians?

derekmorr · 2017-05-12T12:51:10Z

Sure that would work. Do you want to add that to this PR or make a new one for it ?

cquiroz · 2017-05-12T12:52:07Z

@paxelord Could you implement the change suggested by @derekmorr and then we can merge

… unit tests

PhilipAxelrod · 2017-05-13T02:51:53Z

@cquiroz @derekmorr I have added the GradsPerSecond back, in addition to deprecation warnings. Thanks a ton for helping me make this pr open source worthy!

cquiroz

Thanks a lot @paxelord for your contribution

PhilipAxelrod force-pushed the rotational-dynamics-units branch from f92dd88 to a9729ce Compare April 30, 2017 17:56

cquiroz reviewed May 1, 2017

View reviewed changes

PhilipAxelrod force-pushed the rotational-dynamics-units branch 3 times, most recently from 373ae33 to 7c49aad Compare May 5, 2017 23:52

cquiroz reviewed May 7, 2017

View reviewed changes

PhilipAxelrod force-pushed the rotational-dynamics-units branch from 7c49aad to ffb8ee7 Compare May 7, 2017 19:59

PhilipAxelrod added 2 commits May 7, 2017 14:48

Add units required for calculations relating to rotational dynamics s…

b9fe5f4

…uch as Torque, MomentOfInertia, and AngularAcceleration. In addition, add multiply methods to Angle and AngularVelocity

Clarify onRadius method in angular quantities, remove implicit conver…

4468209

…sion from Energy to Torque, and simplify code

PhilipAxelrod force-pushed the rotational-dynamics-units branch from ffb8ee7 to 9f76785 Compare May 7, 2017 21:49

cquiroz mentioned this pull request May 8, 2017

Next release 1.3.0 #232

Closed

Add conversion to units relating to rotational dynamics units and add…

4f277d3

… unit tests

PhilipAxelrod force-pushed the rotational-dynamics-units branch from 9f76785 to 4f277d3 Compare May 13, 2017 02:51

cquiroz approved these changes May 13, 2017

View reviewed changes

cquiroz merged commit 6e3408d into typelevel:master May 13, 2017

This was referenced May 13, 2017

Annotation fixes #236

Merged

Add SI, Troy, US Customary unit groups #235

Merged

derekmorr mentioned this pull request May 21, 2017

Add new Squants units to squants module pureconfig/pureconfig#233

Closed

vlovgr mentioned this pull request May 29, 2017

Upgrade squants to 1.3.0 vlovgr/ciris#16

Merged


		abstract class StrictlyPositiveQuantity[A <: StrictlyPositiveQuantity[A]](value: Double) extends Quantity[A] {self: A ⇒

Rotational dynamics units #227

Rotational dynamics units #227

Conversation

PhilipAxelrod commented Apr 30, 2017

cquiroz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilipAxelrod May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilipAxelrod May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilipAxelrod May 1, 2017 • edited Loading

Choose a reason for hiding this comment

PhilipAxelrod May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilipAxelrod May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cquiroz commented May 7, 2017

cquiroz commented May 12, 2017

derekmorr commented May 12, 2017

cquiroz commented May 12, 2017

derekmorr commented May 12, 2017

cquiroz commented May 12, 2017

PhilipAxelrod commented May 13, 2017 • edited Loading

cquiroz left a comment

Choose a reason for hiding this comment

PhilipAxelrod May 1, 2017 •

edited

Loading

PhilipAxelrod May 1, 2017 •

edited

Loading

PhilipAxelrod May 1, 2017 •

edited

Loading

PhilipAxelrod May 1, 2017 •

edited

Loading

PhilipAxelrod May 1, 2017 •

edited

Loading

PhilipAxelrod commented May 13, 2017 •

edited

Loading