Frequency Enhancements #213

garyKeorkunian · 2017-03-30T22:01:09Z

Added /(Time) and per(Time) to Squantified numbers to support expression like:

val f: Frequency = 1000 / second
val f: Frequency = 1000 per second

Added *(Frequency) to TimeIntegral to support conversions like:

val dr: DataRate = 1000.bytes * 10/second

Added *(TimeIntegral) to Frequency to support the same conversion with reversed operands:

val dr: DataRate = 10/second * 1000.bytes

However, this last expression returns type Any and needs to be reviewed for ideas on how to fix that before this is merged.

This is intended to close #134

garyKeorkunian · 2017-04-02T19:00:42Z

It may be that the Frequency.*(TimeIntegral) method is too abstract and I'll need to define a method for each TimeIntegral in the libary ....

class Frequency(...) {
   ...
   def *(that: Length): Velocity = that * this
   def *(that: Velocity): Acceleration = that * this
   def *(that: Information): DataRate = that * this
   // etc ...
}

It would be nice if the TimeIntegral/Derivative abstraction could handle that, but maybe not.

Thoughts?

garyKeorkunian · 2017-04-04T22:17:01Z

I replaced the Frequency * TimeIntegral code, which was limited, with explicit methods for each defined TimeIntegral. This achieves the API requirement, but with a bit more code.

This could be improved in the future, but this PR should be mergeable as is - pending review.

derekmorr

Just a few small issues, but this look good.

derekmorr · 2017-04-05T01:56:03Z

shared/src/main/scala/squants/time/Frequency.scala

@@ -26,6 +30,36 @@ final class Frequency private (val value: Double, val unit: FrequencyUnit)
  protected[squants] def timeIntegrated = Each(toHertz)
  protected[squants] def time = Seconds(1)

+  // Below are two failed attempts to invert the TimeIntegral * Frequency method to support Frequency * TimeIntegral


I think we should remove the comments from lines 33 - 44 into the PR. I'd like to preserve them, but I don't think they need to be in the source.

derekmorr · 2017-04-05T01:56:55Z

shared/src/test/scala/squants/QuantitySpec.scala

@@ -10,7 +10,7 @@ package squants

 import org.scalatest.{ FlatSpec, Matchers }
 import squants.thermal.{ Celsius, Fahrenheit }
-import squants.time.Hours
+import squants.time.{ Hours, Hertz }


nit - Hertz should be before Hours to keep imports alphabetized.

derekmorr · 2017-04-05T02:06:09Z

I agree spelling out the specific types is sub-optimal, but at least we're type safe. We might be able to implement it more generically in the future at the cost of binary compatibility.

With this new syntax, we should update the README. I had removed this syntax in #202 a few months ago. Can you update shared/src/main/tut/README.me and readd

val freq = 60 / second

example in the "Implicit conversion support for using Double on the left side of multiplication" section (which we probably should rename now).

(this was the original line https://github.com/typelevel/squants/pull/202/files#diff-04c6e90faac2675aa89e2176d2eec7d8L487)

derekmorr · 2017-04-05T02:19:29Z

shared/src/test/scala/squants/time/FrequencySpec.scala

+    Hertz(100) * Webers(100) should be(Volts(10000))
+    Hertz(100) * Kilograms(100) should be(KilogramsPerSecond(10000))
+    Hertz(100) * NewtonSeconds(100) should be(Newtons(10000))
+    Hertz(100) * Watts(100) should be(WattsPerHour(10000))


Why is this in WattsPerHour? Shouldn't the unit be watts per second?

Good catch. Most time derivative relationships are based on the second. Energy/Power/PowerRamp prefer the WattHour, Watt and WattsPerHour units, so the base time is one hour. The new conversion assumed 1 second, which coupled with my blind paste and copy test setup, led to a passing test on a bad conversion. It's fixed up now.

Thanks!

Although WattsPerSecond, which is effectively a joule / sec^2, would be a good addition to library.

derekmorr · 2017-04-06T03:04:41Z

README.md

@@ -788,14 +789,21 @@ scala> val low = 40.dollars / megawattHour
 low: squants.market.Price[squants.energy.Energy] = 40.00 USD/1.0 MWh
 ```

-Implicit conversion support for using Double on the left side of multiplication:
+Implicit conversion support for using Doubles, Longs and BigDecimals on the left side of multiply and divide operations:


It looks like you edited README.md directly. The readme is built via tut now, so these changes will be overwritten on the next build. Can you also update shared/src/main/tut/README.md so they're preserved?

Ah, yes. Thanks. Fixed.

One thing I noticed is

BigDecimal(1) / hour works in the tut, but
BigDecimal(1) per hour does not: method not defined.

However, both of these are defined in the same implicit class and both work in the console. For the README, I just went back to using the / operator. Any thoughts on that?

cquiroz

Looks good to me, thanks for the PR

cquiroz · 2017-04-06T16:18:48Z

shared/src/main/scala/squants/time/Frequency.scala

+  def *(that: LuminousEnergy): LuminousFlux = that * this
+  def *(that: LuminousExposure): Illuminance = that * this
+  def *(that: MagneticFlux): ElectricPotential = that * this
+  def *(that: Mass): MassFlow = that * this


Will this produce e.g. [kgs/sec]

Yes, it will.

[T^-1] * [M] = [M/T]

Added /(Time) and per(Time) to Squantified numbers to support expression like: val f: Frequency = 1000 / second Added *(Frequency) to TimeIntegral to support conversions like: val dr: DataRate = 1000.bytes * 10/second Added *(TimeIntegral) to Frequency to support the same conversion in with reversed operands: val dr: DataRate = 10/second * 1000.bytes However, this returns type Any and needs to be reviewed for ideas on how to fix that. Added *(TimeIntegral) to Frequency for all currently defined TimeIntegrals Review updates * removed extraneous comments * alphabetized imports * updated README * Fixed Power / Energy / Frequency conversions and tests Review updates * Update tut/README * resolved merge conflicts

derekmorr · 2017-04-09T18:27:49Z

LGTM.

derekmorr requested changes Apr 5, 2017

View reviewed changes

derekmorr reviewed Apr 5, 2017

View reviewed changes

derekmorr reviewed Apr 6, 2017

View reviewed changes

garyKeorkunian force-pushed the wip-squantified-divide branch from cc388f5 to b0c14c5 Compare April 6, 2017 13:31

cquiroz reviewed Apr 6, 2017

View reviewed changes

garyKeorkunian force-pushed the wip-squantified-divide branch 2 times, most recently from 383dbe8 to 954eb6a Compare April 7, 2017 00:18

garyKeorkunian force-pushed the wip-squantified-divide branch from 954eb6a to 78bf5a2 Compare April 7, 2017 00:20

Merge branch 'master' into wip-squantified-divide

56dbee7

derekmorr approved these changes Apr 9, 2017

View reviewed changes

derekmorr merged commit 67e03c0 into master Apr 9, 2017

derekmorr deleted the wip-squantified-divide branch April 22, 2017 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Frequency Enhancements #213

Frequency Enhancements #213

garyKeorkunian commented Mar 30, 2017 •

edited

Loading

garyKeorkunian commented Apr 2, 2017

garyKeorkunian commented Apr 4, 2017

derekmorr left a comment

derekmorr Apr 5, 2017

derekmorr Apr 5, 2017

derekmorr commented Apr 5, 2017

derekmorr Apr 5, 2017

garyKeorkunian Apr 5, 2017

garyKeorkunian Apr 5, 2017 •

edited

Loading

derekmorr Apr 6, 2017

garyKeorkunian Apr 6, 2017

garyKeorkunian Apr 6, 2017

cquiroz left a comment

cquiroz Apr 6, 2017

garyKeorkunian Apr 6, 2017 •

edited

Loading

derekmorr commented Apr 9, 2017

Frequency Enhancements #213

Frequency Enhancements #213

Conversation

garyKeorkunian commented Mar 30, 2017 • edited Loading

garyKeorkunian commented Apr 2, 2017

garyKeorkunian commented Apr 4, 2017

derekmorr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekmorr commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyKeorkunian Apr 5, 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

cquiroz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyKeorkunian Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

derekmorr commented Apr 9, 2017

garyKeorkunian commented Mar 30, 2017 •

edited

Loading

garyKeorkunian Apr 5, 2017 •

edited

Loading

garyKeorkunian Apr 6, 2017 •

edited

Loading