[WIP] Support a rational type #146

erikerlandson · 2020-06-10T17:10:58Z

Addresses #140

erikerlandson · 2020-06-10T17:18:04Z

This initial push isn't working. In broad strokes, it seems to be operating the way we were targeting in the #140 discussion: it will attempt to typecheck the implicitly[OpIntercept ... in the macro. Additionally, a direct invocation of implicitly[OpIntercept[OpId.+, Fraction[1,3], Fraction[1,3], 0]] will work, but implicitly[+[Fraction[1,3], Fraction[1,3]]] is ultimately failing with "Cannot extract value from ..." abort error.

Fix may be simple, but thought I'd push this just for the record.

soronpo · 2020-06-10T21:40:09Z

I currently don't have time to get into this, but from a brief read I think I didn't make my point across. Please reread my comments from #140 (comment)

You should not need a specific trait Add (Sub, etc...) here. The macro should see a +[Frac1, Frac2] as an unknown operation (since composed + with the unknown fractional arguments) and redirect a request to an implicitly[AlternativeOp[N, Arg1, Arg2, Arg3]].

soronpo · 2020-06-10T21:43:10Z

src/main/scala/singleton/ops/impl/GeneralMacros.scala

-      (a, b) match {
-        case (aArg : CalcVal, bArg : CalcVal) => abort(s"Unsupported $funcType[$a, $b, $cArg]")
-        case _ => CalcUnknown(funcType.toType, None)
+      val itree = c.typecheck(q"implicitly[_root_.singleton.ops.impl.OpIntercept[${funcType.toType}, ${a.tpe}, ${b.tpe}, ${cArg.tpe}]]", silent = true)


There should be a try-catch here.

I am checking for EmptyTree instead

erikerlandson · 2020-06-10T21:44:09Z

@soronpo that is how it's working. Add is just an implementation detail for how implicitly[OpIntercept[+, Fraction... is actually resolved

soronpo · 2020-06-10T21:45:58Z

@soronpo that is how it's working. Add is just an implementation detail for how implicitly[OpIntercept[+, Fraction... is actually resolved

Oh, right. As I said, a brief look and I missed it. Sorry.

erikerlandson · 2020-06-10T21:46:01Z

Think I may have localized the problem to needing a new case in usingFuncName

soronpo · 2020-06-10T21:47:22Z

Sorry that I can't help right now. I hope I can take a look during the weekend.

soronpo · 2020-06-10T21:51:29Z

If it's not too much bother, I would rather this PR be split into two. The OpIntercept will be the first PR, with a simple example to test it (and the variant failure possibilities). The second PR will add the Fractional under singleton.ops.fractional.

erikerlandson · 2020-06-10T21:54:33Z

No, that's fine. It would probably be easier to debug this with a simpler test case to start with.

erikerlandson · 2020-06-10T22:52:30Z

factored the basic machinery in OpMacro out to #147

…ng it

erikerlandson · 2020-06-24T00:52:10Z

@soronpro I rebased this off of #149 and refactored it. It is still WIP but the basic operations + - * / are working.

soronpo · 2020-06-24T10:45:30Z

Why do you use an upper-bound <: XInt? One of the great things I like about singleton-ops is composition. Upper-bounds prevent this. This also burdens the code with the auxiliary types. Without bounds the code should work the same for Long without any changes.

soronpo · 2020-06-24T11:07:07Z

Looking at the code got me thinking about the way we write the operations in types vs how we write them with type classes.

trait Rational {
  def + [T](t : T)(implicit r : T => Rational) = ???
}

Maybe we should take the same approach with OpIntercept for infix operations, which would be like OpIntercept[L + OpConv[T, L]].

erikerlandson · 2020-06-24T13:08:57Z

Why do you use an upper-bound <: XInt?

Partly it was leftover from the Libra code. Also, not sure what it means to allow a Rational with, say, Float numerator and denominator, or String, etc. Or mixed types. I suppose the underlying numeric operations themselves would fail and constrain it that way.

erikerlandson · 2020-06-24T13:25:37Z

@soronpo another policy question is whether this is consistent with the rest of the library:

      rat: Require[IsRational[LHS] || IsRational[RHS]],
      lhs: OpGen.Aux[ToRational[LHS], Rational[LN, LD]],
      rhs: OpGen.Aux[ToRational[RHS], Rational[RN, RD]],

That allows any argument convertable to Rational to be mixed with a rational arg, like 1 + Rational[1,2]. However, there are some considerations with that. (1) the rest of the library seems to forbid mixed args: 1 + 1D is an error. (2) while this is fine in isolation, it might interact unpredictably with other custom extensions.

I'm starting to think it's better to just require left and right arguments to be both Rational. I want to mix with integers for my own applications, but I can support that myself.

soronpo · 2020-06-24T13:50:29Z

Look at this example. Typelevel operations can be done like typeclasses 😃

  type OpConv[F <: Function1[_,_], Out] = OpIntercept.Aux[F, Out]

  implicit def VecToVec[A0, A1] : OpIntercept.Aux[Vec[A0, A1] => Vec[_, _], Vec[A0, A1]] =
    new OpIntercept[Vec[A0, A1] => Vec[_,_]] {
      val value : Out = new Vec[A0, A1]{}
      type Out = Vec[A0, A1]
    }
  implicit def ScalarIntToVec[I <: XInt] : OpIntercept.Aux[I => Vec[_, _], Vec[I, I]] =
    new OpIntercept[I => Vec[_,_]] {
      val value : Out = new Vec[I, I]{}
      type Out = Vec[I, I]
    }

  implicit def `Vec+`[T, VL0, VL1, VR0, VR1, V[_,_]](
    implicit
    conv : OpConv[T => Vec[_, _], V[VR0, VR1]],
    opL : VL0 + VR0,
    opR : VL1 + VR1
  ) : OpIntercept.Aux[Vec[VL0, VL1] + T, Vec[opL.Out, opR.Out]] = 
    new OpIntercept[Vec[VL0, VL1] + T] {
      type Out = Vec[opL.Out, opR.Out]
      val value : Out = new Vec[opL.Out, opR.Out]{}
    }

  val vecPlusScalar = shapeless.the[Vec[1, 2] + 10] //yeah, I know in math this is not right
  val vecPlusVec    = shapeless.the[Vec[1, 2] + Vec[10, 10]]

soronpo · 2020-06-24T14:16:25Z

That allows any argument convertable to Rational to be mixed with a rational arg, like 1 + Rational[1,2]. However, there are some considerations with that. (1) the rest of the library seems to forbid mixed args: 1 + 1D is an error. (2) while this is fine in isolation, it might interact unpredictably with other custom extensions.

At the beginning, the library allowed that, but that caused the case-statement to be HUGE, and slowed things down. However, with this "typeclass"-like trick, I can add support for extended support for primitive through OpIntercept and the conversion.

erikerlandson · 2020-06-24T14:24:23Z

I can add support for extended support for primitive through OpIntercept and the conversion.

What is your opinion on the issue of conversion interactions? For example, if I write rules that allow various numeric types to convert to Rational, and then write my + so it will accept any two args convertable to Rational, but then someone else does the same for Vec like in your example, and I have both sets of implicits, imported, what happens? In some applications, this can be resolved via output type expected, but in singleton-ops, the output type is not assumed anywhere, so this seems like a recipe for divergent implicit matching errors

erikerlandson · 2020-06-24T14:31:35Z

requiring something like rat: Require[IsRational[LHS] || IsRational[RHS]] may sufficiently disambiguate things but implicit matching is tricky to reason about when possible conversions are also in the mix, and I don't feel a lot of certainty about what happens when multiple customizations that include argument casting start interacting.

soronpo · 2020-06-24T14:36:18Z

I can add support for extended support for primitive through OpIntercept and the conversion.

What is your opinion on the issue of conversion interactions? For example, if I write rules that allow various numeric types to convert to Rational, and then write my + so it will accept any two args convertable to Rational, but then someone else does the same for Vec like in your example, and I have both sets of implicits, imported, what happens? In some applications, this can be resolved via output type expected, but in singleton-ops, the output type is not assumed anywhere, so this seems like a recipe for divergent implicit matching errors

You need to have a separation:
A <: Rational + T => Rational
and
T => Rational + A <: Rational

In case both side are rational, maybe just implicit prioritization will do.
It's the same as doing it with terms (if you have two generic extension methods + in scope) .

erikerlandson · 2020-06-24T14:49:10Z

@soronpo so you'd prefer an idiom like conv : OpConv[T => Rational[_, _], Rational[N, D]] instead of OpGen.Aux[ToRational[T], Rational[N, D]] ?

soronpo · 2020-06-24T14:59:55Z

@soronpo so you'd prefer an idiom like conv : OpConv[T => Rational[_, _], Rational[N, D]] instead of OpGen.Aux[ToRational[T], Rational[N, D]] ?

Yes, because it saves you from needing to declare a new type/trait

soronpo · 2020-06-24T15:00:39Z

That is, unless the user requires an explicit ToRational

erikerlandson · 2020-06-24T15:03:47Z

That is, unless the user requires an explicit ToRational

I see that as a library policy convention question - do you want a ToRational, or do you want users to use => ? I assume ToRational could be a thin wrapper around =>

erikerlandson · 2020-06-24T15:19:36Z

The idiom OpConv[T => Float, FloatVal] isn't really any more wordy than OpFloat.Aux[ToFloat[T], FloatVal]. I can imagine updating the entire library conventions around type-casting to =>

soronpo · 2020-06-24T15:28:27Z

The idiom OpConv[T => Float, FloatVal] isn't really any more wordy than OpFloat.Aux[ToFloat[T], FloatVal]. I can imagine updating the entire library conventions around type-casting to =>

They don't do the same thing (conversion vs. just an auxiliary).
BTW, please use OpAuxGen, OpAuxInt, etc. which are directly available under singleton.ops. The impl namespace was meant to be internal and I didn't set it as protected by mistake.

erikerlandson · 2020-06-24T15:29:39Z

They don't do the same thing (conversion vs. just an auxiliary).

I do not understand this distinction

soronpo · 2020-06-24T15:30:38Z

They don't do the same thing (conversion vs. just an auxiliary).

I do not understand this distinction

One is .toInt while the other is .asInstanceOf[Int], but within the typelevel world

soronpo · 2020-06-24T15:34:23Z

The only reason you require OpCast derivatives such as OpAuxInt is when using upper-bounds. Because the default .Out is not upper-bounded and if you wish to feed it to an upper-bounded type, then you're stuck without that upper-bound casting.

erikerlandson · 2020-06-24T16:56:31Z

One is .toInt while the other is .asInstanceOf[Int], but within the typelevel world
The only reason you require OpCast derivatives such as OpAuxInt is when using upper-bounds.

OK that is clear. So, my current use of ToRational is a misnomer, since it isn't really just applying an upper bound, it is more like .toRational instead of .asInstanceOf[Rational]

With this new support for custom non-singleton types, it might be useful to have something like a generic ToXxx function, for applying upper bounds. Something like OpAuxCast[Rational[_,_], ...]

soronpo · 2020-06-24T17:09:41Z

With this new support for custom non-singleton types, it might be useful to have something like a generic ToXxx function, for applying upper bounds. Something like OpAuxCast[Rational[_,_], ...]

There is impl.OpCast, and we can alias it to be used as you mention, but this is not true Rational[1,2] <: Rational[_,_] when we use invariance. And if we try to apply variance, this will screw with the singleton narrowness of the values.

erikerlandson · 2020-06-24T17:17:48Z

And if we try to apply variance, this will screw with the singleton narrowness of the values.

If extending casting to non-singleton types breaks the model, it maybe isn't worth it

erikerlandson · 2020-06-24T17:50:23Z

Well, actually ToXxx isn't just "as-instance" it is conversion:

scala> shapeless.the[ToFloat[1]].value
res2: Float = 1.0

So the entire ToXxx suite could be replaced by =>

soronpo · 2020-06-24T18:33:40Z

Well, actually ToXxx isn't just "as-instance" it is conversion:
scala> shapeless.the[ToFloat[1]].value
res2: Float = 1.0
So the entire ToXxx suite could be replaced by =>

But ToXXxx are explicit conversions and composable. Maybe there is a way to merge them better,

soronpo · 2020-06-24T23:12:10Z

Maybe you're right and it's possible to unify OpConv and OpAuxGen (and OpIntercept). I need to think about this.

soronpo · 2020-06-24T23:49:10Z

requiring something like rat: Require[IsRational[LHS] || IsRational[RHS]] may sufficiently disambiguate things but implicit matching is tricky to reason about when possible conversions are also in the mix, and I don't feel a lot of certainty about what happens when multiple customizations that include argument casting start interacting.

BTW, here is what Long and Int mixing looks like:

  implicit def Long2Long[L <: XLong] : OpConv[L => XLong, L] = ???
  implicit def Int2Long[I <: XInt](implicit op : ToLong[I]) : OpConv[I => XLong, op.Out] = ???

  implicit def `Long+Long`[L, R, LL, RL](  //repeat for all basic operations that support Long
    implicit
    convL : OpConv[L => XLong, LL],
    convR : OpConv[R => XLong, RL],
    op : LL + RL
  ) : OpIntercept.Aux[L + R, op.Out] = ???

  implicitly[1 + 1L + 1]
  implicitly[1L + 1 + 1L]

erikerlandson · 2020-06-25T00:09:03Z

BTW, here is what Long and Int mixing looks like

What should the cross-product of operator output types look like? For example, the following can all lose integer precision on the left hand side:

int + float
long + float
long + double

The following are reasonable and defensible, but aren't lossless:

int + float -> float(lhs)) + rhs
long + float -> float(lhs)) + rhs
long + double -> double(lhs) + rhs

erikerlandson · 2020-06-25T00:10:29Z

btw, I am in the process of removing T <: XInt bounds, moving to OpAuxGen, etc

soronpo · 2020-06-25T00:25:06Z

BTW, here is what Long and Int mixing looks like

What should the cross-product of operator output types look like? For example, the following can all lose integer precision on the left hand side:
* int + float

* long + float

* long + double

The same as in terms. That's why it's best to have these implicits in a separate namespace.
Although, since we are in type-level, we can disallow conversions that loose precision.

We can have:
import singleton.ops.conversions.basic._ for the related int/long/char/string/boolean combinations.
import singleton.ops.conversions.floating._ for the float/double combinations.

erikerlandson · 2020-06-25T00:34:51Z

Although, since we are in type-level, we can disallow conversions that loose precision.

I've done similar policy settings using implicit values:
https://github.com/erikerlandson/coulomb/blob/develop/coulomb/src/main/scala/coulomb/package.scala#L54

and then using them is like:
https://github.com/erikerlandson/coulomb/blob/develop/coulomb/src/main/scala/coulomb/infra/canonical.scala#L97

soronpo · 2020-06-25T01:25:32Z

Nice way to support various casting combinations 😄

  sealed trait Cast
  object Cast {
    trait Char extends Cast
    trait Int extends Cast
    trait Long extends Cast
    trait Float extends Cast
    trait Double extends Cast
    trait Boolean extends Cast
    trait String extends Cast
  }
  implicit def castCI[L <: XChar,   R <: XInt]  : OpConv[(L, R) => Cast, Cast.Int] = ???
  implicit def castCL[L <: XChar,   R <: XLong] : OpConv[(L, R) => Cast, Cast.Long] = ???
  implicit def castIC[L <: XInt,    R <: XChar] : OpConv[(L, R) => Cast, Cast.Int] = ???
  implicit def castIL[L <: XInt,    R <: XLong] : OpConv[(L, R) => Cast, Cast.Long] = ???
  implicit def castLC[L <: XLong,   R <: XChar] : OpConv[(L, R) => Cast, Cast.Long] = ???
  implicit def castLI[L <: XLong,   R <: XInt]  : OpConv[(L, R) => Cast, Cast.Long] = ???

  implicit def Char2Char[C <: XChar] : OpConv[C => Cast.Char, C] = ???
  implicit def Char2Int[C <: XChar](implicit op : ToInt[C]) : OpConv[C => Cast.Int, op.Out] = ???
  implicit def Char2Long[C <: XChar](implicit op : ToLong[C]) : OpConv[C => Cast.Long, op.Out] = ???
  implicit def Int2Int[I <: XInt] : OpConv[I => Cast.Int, I] = ???
  implicit def Int2Long[I <: XInt](implicit op : ToLong[I]) : OpConv[I => Cast.Long, op.Out] = ???
  implicit def Long2Long[L <: XLong] : OpConv[L => Cast.Long, L] = ???

  implicit def `+Ext`[L, R, BO, LL, RL](
    implicit
    basicOp : OpConv[(L, R) => Cast, BO],
    convL : OpConv[L => BO, LL],
    convR : OpConv[R => BO, RL],
    op : LL + RL
  ) : OpIntercept.Aux[L + R, op.Out] = ???

  implicit def `-Ext`[L, R, BO, LL, RL](
    implicit
    basicOp : OpConv[(L, R) => Cast, BO],
    convL : OpConv[L => BO, LL],
    convR : OpConv[R => BO, RL],
    op : LL - RL
  ) : OpIntercept.Aux[L - R, op.Out] = ???

  implicit def `*Ext`[L, R, BO, LL, RL](
    implicit
    basicOp : OpConv[(L, R) => Cast, BO],
    convL : OpConv[L => BO, LL],
    convR : OpConv[R => BO, RL],
    op : LL * RL
  ) : OpIntercept.Aux[L * R, op.Out] = ???

  implicit def `/Ext`[L, R, BO, LL, RL](
    implicit
    basicOp : OpConv[(L, R) => Cast, BO],
    convL : OpConv[L => BO, LL],
    convR : OpConv[R => BO, RL],
    op : LL / RL
  ) : OpIntercept.Aux[L / R, op.Out] = ???

  implicitly[20/4 + 1L + 1 * 5L + 'c']

erikerlandson · 2020-06-25T14:18:56Z

@soronpo I updated the code to remove any XInt upper bounds. It is moving in the right direction, for example the following now operates using either XInt or XLong:

scala> shapeless.the[Simplify[Rational[4,6]]].value.show
res0: String = Rational(2, 3)

scala> shapeless.the[Simplify[Rational[4L,6L]]].value.show
res1: String = Rational(2, 3)

However some invocations are now failing, although superficially they should work exactly as before. These are two different failures, that may or may not be related:

scala> shapeless.the[Simplify[Rational[-4,6]]].value.show
                    ^
       error: Simplify requires non-zero denominator

scala> shapeless.the[Rational[1,2] + Rational[1,3]].value.show
                    ^
       error: Calculation has returned a non-literal type/value.
       To accept non-literal values, use `AcceptNonLiteral[T]`.

erikerlandson · 2020-06-25T15:13:24Z

it looks like AcceptNonLiteral has some overlap with OpIntercept, at least in the sense of being a way to extract non-literal values out of an operation

soronpo · 2020-06-25T15:22:51Z

You shouldn't need to use AcceptNonLiteral in your context.
I admit I may need to rethink how to implement AcceptNonLiteral better in the future.

erikerlandson · 2020-06-25T15:28:24Z

You shouldn't need to use AcceptNonLiteral in your context.

The latest code is pushed. I don't yet understand why backing away from the XInt type bounds causes things to start failing. "Low level" calls like GCD appear to work, but non-basis Simplify is not, so I'm wondering if it is an implicit resolution stack-depth issue. GCD is also all OpAuxGen now, and so it wouldn't seem to be an issue of just removing type bounds per se.

soronpo · 2020-06-25T15:39:19Z

I may have found a way to combine OpIntercept, OpConv and OpAuxGen, so maybe things will be solved after.
Regarding your implementation, I will check it after I do.

erikerlandson · 2020-07-23T17:07:45Z

I may have found a way to combine OpIntercept, OpConv and OpAuxGen, so maybe things will be solved after.

@soronpo have you had a chance to play with this?

soronpo · 2020-07-24T09:26:28Z

@erikerlandson I've pushed my WIP to the other pull request

soronpo reviewed Jun 10, 2020

View reviewed changes

Oron Port and others added 9 commits June 21, 2020 00:35

Add OpIntercept to allow custom types and operations

2277a09

Assume caching to be unique to the running operation without specifyi…

a56f62b

…ng it

fix calculation missing unknown return

14d3c9f

Added error cases coverage and fixed literals for pre-2.13 scala

4daee9a

fix minor issue

b87d834

changed method of procuring the Out for OpIntercept

6a278b7

add value support

aa14d5e

fixed test-case for scala pre-2.13

6e5868a

initial draft for Rational support

a9db1b8

erikerlandson force-pushed the rational-140 branch from 979e441 to a9db1b8 Compare June 24, 2020 00:48

remove type bounds for GCD

14abaa4

abstract away from XInt, use OpAuxGen

eedb630

[WIP] Support a rational type #146

Are you sure you want to change the base?

[WIP] Support a rational type #146

Conversation

erikerlandson commented Jun 10, 2020

erikerlandson commented Jun 10, 2020

soronpo commented Jun 10, 2020 • edited Loading

soronpo Jun 10, 2020

Choose a reason for hiding this comment

erikerlandson Jun 10, 2020

Choose a reason for hiding this comment

erikerlandson commented Jun 10, 2020

soronpo commented Jun 10, 2020

erikerlandson commented Jun 10, 2020

soronpo commented Jun 10, 2020

soronpo commented Jun 10, 2020

erikerlandson commented Jun 10, 2020

erikerlandson commented Jun 10, 2020

erikerlandson commented Jun 24, 2020

soronpo commented Jun 24, 2020

soronpo commented Jun 24, 2020

erikerlandson commented Jun 24, 2020

erikerlandson commented Jun 24, 2020

soronpo commented Jun 24, 2020 • edited Loading

soronpo commented Jun 24, 2020

erikerlandson commented Jun 24, 2020

erikerlandson commented Jun 24, 2020

soronpo commented Jun 24, 2020 • edited Loading

erikerlandson commented Jun 24, 2020

soronpo commented Jun 24, 2020

soronpo commented Jun 24, 2020

erikerlandson commented Jun 24, 2020

erikerlandson commented Jun 24, 2020

soronpo commented Jun 24, 2020 • edited Loading

erikerlandson commented Jun 24, 2020

soronpo commented Jun 24, 2020 • edited Loading

soronpo commented Jun 24, 2020

erikerlandson commented Jun 24, 2020 • edited Loading

soronpo commented Jun 24, 2020 • edited Loading

erikerlandson commented Jun 24, 2020

erikerlandson commented Jun 24, 2020

soronpo commented Jun 24, 2020

soronpo commented Jun 24, 2020

soronpo commented Jun 24, 2020

erikerlandson commented Jun 25, 2020 • edited Loading

erikerlandson commented Jun 25, 2020

soronpo commented Jun 25, 2020

erikerlandson commented Jun 25, 2020

soronpo commented Jun 25, 2020

erikerlandson commented Jun 25, 2020

erikerlandson commented Jun 25, 2020

soronpo commented Jun 25, 2020

erikerlandson commented Jun 25, 2020 • edited Loading

soronpo commented Jun 25, 2020

erikerlandson commented Jul 23, 2020

soronpo commented Jul 24, 2020

soronpo commented Jun 10, 2020 •

edited

Loading

soronpo commented Jun 24, 2020 •

edited

Loading

soronpo commented Jun 24, 2020 •

edited

Loading

soronpo commented Jun 24, 2020 •

edited

Loading

soronpo commented Jun 24, 2020 •

edited

Loading

erikerlandson commented Jun 24, 2020 •

edited

Loading

soronpo commented Jun 24, 2020 •

edited

Loading

erikerlandson commented Jun 25, 2020 •

edited

Loading

erikerlandson commented Jun 25, 2020 •

edited

Loading