add implicitly chain for angle conversion factors #52

ldrygala · 2017-12-10T14:43:31Z

add inductive implicit for chaining angle conversion factors, as referenced in #50

codecov-io · 2017-12-10T20:30:32Z

Codecov Report

Merging #52 into master will decrease coverage by 0.58%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
- Coverage   85.82%   85.24%   -0.59%     
==========================================
  Files          10       10              
  Lines         254      244      -10     
==========================================
- Hits          218      208      -10     
  Misses         36       36

Impacted Files	Coverage Δ
src/main/scala/libra/ops/base.scala	`100% <100%> (ø)`	⬆️
src/main/scala/libra/nonsi/package.scala	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a2b281...a296c30. Read the comment docs.

zainab-ali · 2017-12-13T13:35:54Z

src/main/scala/libra/ops/base.scala

  }

+  object ConversionFactor {
+    implicit def inductiveAngelConversionFactor[A, From <: UnitOfMeasure[Angle], To <: UnitOfMeasure[Angle], Next <: UnitOfMeasure[Angle]](


Could this part be generic on any dimension D instead of Angle?

i tried but i didn't work (diverging implicit expansion for type libra.ops.base.ConversionFactor), therefor i used Angle. I will try one more time to make it more generic

That's ok, there's no need if you already tried! I'll take a better look at it later this week. I've debugged a fair few implicit resolution errors by now, so may have more luck.

Ok, so i'm waiting for your solution and i hope that i'll be able to understand them :)

zainab-ali · 2017-12-13T13:36:48Z

src/main/scala/libra/ops/base.scala

+    implicit def inductiveAngelConversionFactor[A, From <: UnitOfMeasure[Angle], To <: UnitOfMeasure[Angle], Next <: UnitOfMeasure[Angle]](
+      implicit multiplicativeSemigroup: MultiplicativeSemigroup[A],
+      fromConversion: ConversionFactor[A, Angle, From, Next],
+      toConversion: Lazy[ConversionFactor[A, Angle, Next, To]]


zainab-ali · 2017-12-13T13:45:25Z

src/main/scala/libra/ops/base.scala

  }

+  object ConversionFactor {
+    implicit def inductiveAngelConversionFactor[A, From <: UnitOfMeasure[Angle], To <: UnitOfMeasure[Angle], Next <: UnitOfMeasure[Angle]](


Typo in 'Angel' here

zainab-ali · 2017-12-13T13:47:20Z

Thanks @ldrygala this looks awesome! It removes the otherwise exponential boilerplate.

Sorry for not getting round to this earlier - I had a nasty cold and couldn't code for a few days.

Would it be possible to make it even more generic in dimension?
Other than that, I'm really looking forward to merging this in! 👍

ldrygala · 2017-12-14T13:10:11Z

@zainab-ali i removed unused compose function on ConversionFactor, previously we use it to compose bigger ConversionFactor base on smaller. Would you like to merge this PR in current form or would you like to make ConversionFactor more generic in dimension ?

zainab-ali · 2017-12-15T21:49:16Z

@ldrygala I'd like to sort out the implicit resolution issues for the general case. I don't think we're too far off a solution for this.

I've been investigating why there are implicit resolution issues with general dimensions, other than Angle.

What we have is a graph of potential conversions, and we'd like the compiler to pick out a specific path.
In the case of Angle there is only a single possible path, so the compiler can find the appropriate conversion. In the case of SI units, we provide multiple implicit conversions from one point to another using the metricConversion function. This converts from any unit to any other by examining the decimal power and performing an appropriate multiplication. I suspect that if we got rid of this function, and replaced it with one that only derived a factor for P to P + 1 then the inductive derivation would work.

However, even if the derivation works, the approach isn't ideal. This is mainly because:

Graph like implicit searches are seen as a bug, and aren't something that the compiler should be doing. They could very well be removed in future versions.
The diverging implicit error obtained when there are multiple paths is nonsensical. I've seen messages like:

diverging implicit expansion for type libra.ops.base.ConversionFactor[Double,libra.si.Time,libra.si.Metre,Next]

@milessabin suggested the interesting alternative of providing evidence in the form of a HList path of conversions:

// This can only be created with appropriate conversions from each node
implicit val conversionPath : ConversionPath[Degree :: ArcMinute :: ArcSecond :: HNil] = mkConversionPath

Deriving conversion factors would then be a matter of finding two nodes in the path and traversing all the nodes in between.

This would pose a problem in cases where we wanted to define different units for the same base dimensions in different packages.
For example, a user may want to import SI time units, but leave out the units in the imperial package. We could have two paths:

implicit val conversionPathMetric : ConversionPath[Second :: Millisecond :: _ :: HNil] = mkConversionPath
implicit val conversionPathDay : ConversionPath[Second :: Hour :: Day :: HNil] = mkConversionPath

And then somehow relate the two using a lower priority implicit.

This would be a bit more effort, but would provide better messages in the long run.

I'll experiment with this over the next few days, and see if I can come to a solution on it.

ldrygala · 2017-12-16T16:00:38Z

@zainab-ali very interesting idea with conversionPath represented as HList. Is there anything I can get involved with this PR ?

zainab-ali · 2017-12-16T17:58:39Z

Would you like to have a go at it? It involves some fairly heavy induction, which is not everyone's cup of tea, believe me.

The steps I'm envisaging are as follows:

Create a path from a series of base conversion factors. I've done this below
Create derived conversion factors from the path. This involves, for any units From and To:

a. Getting the indices of the units in the path. Ideally shapeless contains an IndexOf op we can use
b. Extracting a range of conversion factors between the units. shapeless contains a Range op that we can use here
c. Multiplying / dividing all of the factors together. Whether we use multiplication or division depends on the order in which the units are defined.
Extrapolating this for units defined over different paths.

Creating the path is fairly straightforward:

package libra
package ops

import base.ConversionFactor
import shapeless._
import shapeless.ops.hlist._

trait ConversionPath[A, D, H <: HList] {
  type Out <: HList
  val path: Out
}

object ConversionPath {

  type Aux[A, D, H <: HList, Out0 <: HList] = ConversionPath[A, D, H] { type Out = Out0 }
  
  implicit def conversionPathBase[A, D, F <: UnitOfMeasure[D], T <: UnitOfMeasure[D]](
    implicit ev: ConversionFactor[A, D, F, T]
  ): ConversionPath.Aux[A, D, F :: T :: HNil, ConversionFactor[A, D, F, T] :: HNil] =
    new ConversionPath[A, D, F :: T :: HNil] {
      type Out = ConversionFactor[A, D, F, T] :: HNil
      val path = ev :: HNil
    }

  implicit def conversionPathRecurse[A, D, F <: UnitOfMeasure[D], T <: UnitOfMeasure[D],
    Tail <: HList, OutTail <: HList](
    implicit ev: ConversionPath.Aux[A, D, T :: Tail, OutTail],
    ev1: ConversionFactor[A, D, F, T]
  ): ConversionPath.Aux[A, D, F :: T :: Tail, ConversionFactor[A, D, F, T] :: OutTail] =
    new ConversionPath[A, D, F :: T :: Tail] {
      type Out = ConversionFactor[A, D, F, T] :: OutTail
      val path = ev1 :: ev.path
    }
}

Step 2 is more challenging. Let me know if you'd like to give it a try.

ldrygala · 2017-12-16T21:22:47Z

i thought that maybe there will be some easy thing to do, but it looks very hard, so therefore i can only wish you good luck.

zainab-ali · 2018-03-12T21:52:20Z

I've finally finished the package refactor! I'll be able to work on this over the weekend.

Lukasz Drygala added 3 commits December 10, 2017 15:35

add implicitly chain for angle conversion factors

f7453cd

install ruby-dep

546139d

fix accidental refactoring

fa30477

zainab-ali reviewed Dec 13, 2017

View reviewed changes

Lukasz Drygala added 2 commits December 13, 2017 15:36

fix typo

da231c0

remove unused compose function on ConversionFactor

a296c30

add implicitly chain for angle conversion factors #52

Are you sure you want to change the base?

add implicitly chain for angle conversion factors #52

Uh oh!

Conversation

ldrygala commented Dec 10, 2017

Uh oh!

codecov-io commented Dec 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zainab-ali Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

ldrygala Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zainab-ali Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

ldrygala Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

zainab-ali Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

zainab-ali Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

ldrygala Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

zainab-ali commented Dec 13, 2017

Uh oh!

ldrygala commented Dec 14, 2017

Uh oh!

zainab-ali commented Dec 15, 2017

Uh oh!

ldrygala commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zainab-ali commented Dec 16, 2017

Uh oh!

ldrygala commented Dec 16, 2017

Uh oh!

zainab-ali commented Mar 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented Dec 10, 2017 •

edited

Loading

ldrygala Dec 13, 2017 •

edited

Loading

ldrygala commented Dec 16, 2017 •

edited

Loading