Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

simplify coulomb's implicit type system #537

Open
wants to merge 9 commits into
base: scala3
Choose a base branch
from

Conversation

erikerlandson
Copy link
Owner

@erikerlandson erikerlandson commented Nov 18, 2023

This will be to experiment with ideas in #534.

The main simplifications will be:

  1. remove support for value promotion
  2. removal of Add and friends, folding everything back into operator signatures
  3. all implicit conversions via scala's implicit conversion enablement (excepting things like q.toUnit, q.toValue, etc)
  4. coulomb-spire folded into coulomb-core
  5. removal of coulomb's internal Rational (using spire Rational instead) (this may allow removal of coulomb-testkit)
  6. substantial simplification of "policies", possibly removal

before final merge

  • update mima baseline
  • remove policies?
  • scan for unused methods, defs and remove
  • update scaladoc
  • update site doc
  • factor "policy" out of pureconfig, refined
  • pgrep for "policy"
  • add inlined algebra to benchmarking
  • factor Rng -> mult SG and add G?
  • what is this: "Result of implicit search for pureconfig.ConfigReader[coulomb.RuntimeUnit] will change."

@erikerlandson
Copy link
Owner Author

I expect this to fail CI for a while, as I rewrite it in pieces

@benhutchison
Copy link

benhutchison commented Nov 23, 2023

The one Im not keen on here is

  1. coulomb-spire folded into coulomb-core

This means, using Coulomb would automatically pull in Spire right? Spire is a large and largely unmaintained, or at least auto-pilot, codebase. Much of it written by Non who has been inactive in Scala for years. It contains some fantastic code and ideas but it also spans a very wide range of features in a single monolithic library.

Once a library is a dependency of a project, it can be very hard to prevent usage of it throughout a codebase. So pulling in Spire may create an additional hurdle for people to embrace Coulomb.

Is Rational the main thing Coulomb needs from Spire?

@erikerlandson
Copy link
Owner Author

@benhutchison spire is currently maintained by @armanbilge. To a certain degree I share your concerns. Spire is no longer under active development, OTOH at some point coulomb will be in a state I consider "complete" and at that point it won't really be under active development either. I think the question is whether the typelevel community has some kind of commitment to maintaining Spire.

One might ask the same questions for coulomb, hypothetically. It's certainly not a problem now, but some day off in The Future, I might not want to do this any more 😂 Ideally, if and when that day comes, I'll have handed off the sustaining-engineering to someone in typelevel.

Regarding Rational - that is the only numeric type that is directly used by coulomb, that spire could provide. However, it adds significant overhead. Both the implementation itself, and the unit testing, which requires an entire subproject: coulomb-testkit It also requires me to have separate parallel policies for value conversion, since scala's native conversions do not extend properly outside of native types, or even frankly for BigDecimal and BigInteger - one very nice thing about spire is that it's value conversion system works seamlessly for both its types and scala/java native types.

@armanbilge
Copy link
Contributor

armanbilge commented Nov 24, 2023

@benhutchison as Erik says, I am currently maintaining Spire. We can definitely consider any concrete ideas you have to improve it, without breaking compatibility of course :)

@erikerlandson
Copy link
Owner Author

@armanbilge I figured out how to make summonFrom work for me:

compiletime.summonFrom {

@benhutchison
Copy link

benhutchison commented Nov 26, 2023

I don't think I properly conveyed my concern with depending on Spire. In a nutshell, it's the "kitchen sink" approach - its large, complex and contains a lot of varied stuff. Not bad or low-quality stuff, quite the opposite. But alot of stuff.

The "unmaintained" comment was primarily to imply it has hardened up and seems unlikely to ever undergo modularisation into smaller units. Perhaps unlikely to see the TODO gaps in the documentation ever filled out.

Coulomb may want Rational, but along with that it pulls in a zoo of RNGs, Polynomials, Quaternions.. So if ever anyone wants to use Coulomb for even the simplest use case, they get all this maths machinery in their classpath. In a team env, it can be hard to argue against using classes once they are on classpath.

ATM Coulomb core is a working, useful module that provides significant value without a Spire dependency. I see that as worthwhile preserving if possible.

@erikerlandson
Copy link
Owner Author

@benhutchison possibly "better modularization" is a feature of interest for spire that @armanbilge could consider. I'm not sure what a better modularization would look like, but if it can be done it seems useful.

coulomb already includes typelevel algebra as a dependency, which also includes a lot of definitions that coulomb doesn't need. It's arguably a cost of code reuse. You rarely get just exactly what you need and nothing else.

Frankly I'd be interested in Scala itself adopting spire's design for value conversions. Scala's current system treats BigInt/BigDecimal differently than the core numeric types, which I find annoying.

@armanbilge
Copy link
Contributor

@benhutchison I encourage you to read and comment on this issue:

@benhutchison
Copy link

OK, decision accepted :) You've made your thinking clear @erikerlandson and I appreciate your arguments are reasonable even if personally I probably prefer the "status quo" design.

@erikerlandson
Copy link
Owner Author

I was planning to refactor the constant-factor syntax enhancements, but I don't think any of it works right currently, see:
#541

I am going to just remove all of it. I will put it back, refactored, if scala 3.4 proves to fix it.

@erikerlandson
Copy link
Owner Author

As long as I am committing to onboarding spire, it gives me an opportunity to jettison my non-standard FractionalPower and TruncatedPower and also my hacked implementations of TruncatedDivision. One can get these cleanly via import spire.implicits.given

As part of this process, I have also rewritten pow and tpow using some interesting new scala-3 compiletime features summonFrom and erasedValue

@erikerlandson
Copy link
Owner Author

Using erasedValue allows some very interesting possibilities, but it has a weird quirk where you cannot specify function return types:
scala/scala3#19294

@erikerlandson
Copy link
Owner Author

I wanted to revisit the question of AnyVal versus opaque type

Here is the current opaque type based system, note I am using an erasedValue based optimization for Double (in both cases):

object googoo:
    import syntax.*
    val q1 = 1.0.withUnit[1000]
    val q2 = q1.toUnit[1]

The resulting bytecode looks like this:

  public static {};
    Code:
       0: new           #2                  // class coulomb/googoo$
       3: dup
       4: invokespecial #30                 // Method "<init>":()V
       7: putstatic     #32                 // Field MODULE$:Lcoulomb/googoo$;
      10: getstatic     #35                 // Field coulomb/quantity$package$Quantity$.MODULE$:Lcoulomb/quantity$package$Quantity$;
      13: getstatic     #38                 // Field coulomb/quantity$package$Quantity$Applier$.MODULE$:Lcoulomb/quantity$package$Quantity$Applier$;
      16: invokevirtual #42                 // Method coulomb/quantity$package$Quantity$Applier$.ctx_Applier:()Lcoulomb/quantity$package$Quantity$Applier;
      19: invokevirtual #46                 // Method coulomb/quantity$package$Quantity$.apply:(Lcoulomb/quantity$package$Quantity$Applier;)Lcoulomb/quantity$package$Quantity$Applier;
      22: dconst_1
      23: invokestatic  #52                 // Method scala/runtime/BoxesRunTime.boxToDouble:(D)Ljava/lang/Double;
      26: invokevirtual #55                 // Method coulomb/quantity$package$Quantity$Applier.apply:(Ljava/lang/Object;)Ljava/lang/Object;
      29: invokestatic  #59                 // Method scala/runtime/BoxesRunTime.unboxToDouble:(Ljava/lang/Object;)D
      32: putstatic     #61                 // Field q1:D
      35: getstatic     #66                 // Field coulomb/quantity$package$.MODULE$:Lcoulomb/quantity$package$;
      38: astore_0
      39: getstatic     #35                 // Field coulomb/quantity$package$Quantity$.MODULE$:Lcoulomb/quantity$package$Quantity$;
      42: astore_1
      43: aload_0
      44: astore_2
      45: getstatic     #32                 // Field MODULE$:Lcoulomb/googoo$;
      48: invokevirtual #69                 // Method q1:()D
      51: dstore_3
      52: ldc2_w        #70                 // double 1000.0d
      55: aload_0
      56: astore        7
      58: dload_3
      59: dstore        8
      61: dload         8
      63: dmul
      64: dstore        5
      66: getstatic     #35                 // Field coulomb/quantity$package$Quantity$.MODULE$:Lcoulomb/quantity$package$Quantity$;
      69: getstatic     #38                 // Field coulomb/quantity$package$Quantity$Applier$.MODULE$:Lcoulomb/quantity$package$Quantity$Applier$;
      72: invokevirtual #42                 // Method coulomb/quantity$package$Quantity$Applier$.ctx_Applier:()Lcoulomb/quantity$package$Quantity$Applier;
      75: invokevirtual #46                 // Method coulomb/quantity$package$Quantity$.apply:(Lcoulomb/quantity$package$Quantity$Applier;)Lcoulomb/quantity$package$Quantity$Applier;
      78: dload         5
      80: invokestatic  #52                 // Method scala/runtime/BoxesRunTime.boxToDouble:(D)Ljava/lang/Double;
      83: invokevirtual #55                 // Method coulomb/quantity$package$Quantity$Applier.apply:(Ljava/lang/Object;)Ljava/lang/Object;
      86: invokestatic  #59                 // Method scala/runtime/BoxesRunTime.unboxToDouble:(Ljava/lang/Object;)D
      89: putstatic     #73                 // Field q2:D
      92: return

Now here is an equivalent bit of code using AnyVal:

object hoohoo:
    import qvcsyntax.*
    val q1 = 1.0.asQVC[1000]
    val q2 = q1.toUnit[1]

And its bytecode:

  public static {};
    Code:
       0: new           #2                  // class coulomb/hoohoo$
       3: dup
       4: invokespecial #19                 // Method "<init>":()V
       7: putstatic     #21                 // Field MODULE$:Lcoulomb/hoohoo$;
      10: getstatic     #26                 // Field coulomb/QVC$.MODULE$:Lcoulomb/QVC$;
      13: dconst_1
      14: invokestatic  #32                 // Method scala/runtime/BoxesRunTime.boxToDouble:(D)Ljava/lang/Double;
      17: invokevirtual #36                 // Method coulomb/QVC$.apply:(Ljava/lang/Object;)Ljava/lang/Object;
      20: checkcast     #38                 // class java/lang/Double
      23: putstatic     #40                 // Field q1:Ljava/lang/Double;
      26: getstatic     #21                 // Field MODULE$:Lcoulomb/hoohoo$;
      29: invokevirtual #43                 // Method q1:()Ljava/lang/Double;
      32: astore_0
      33: ldc2_w        #44                 // double 1000.0d
      36: aload_0
      37: invokestatic  #49                 // Method scala/runtime/BoxesRunTime.unboxToDouble:(Ljava/lang/Object;)D
      40: dmul
      41: dstore_1
      42: new           #51                 // class coulomb/QVC
      45: dup
      46: getstatic     #26                 // Field coulomb/QVC$.MODULE$:Lcoulomb/QVC$;
      49: dload_1
      50: invokestatic  #32                 // Method scala/runtime/BoxesRunTime.boxToDouble:(D)Ljava/lang/Double;
      53: invokevirtual #36                 // Method coulomb/QVC$.apply:(Ljava/lang/Object;)Ljava/lang/Object;
      56: invokespecial #54                 // Method coulomb/QVC."<init>":(Ljava/lang/Object;)V
      59: invokevirtual #58                 // Method coulomb/QVC.value:()Ljava/lang/Object;
      62: checkcast     #38                 // class java/lang/Double
      65: putstatic     #60                 // Field q2:Ljava/lang/Double;
      68: return

@erikerlandson
Copy link
Owner Author

erikerlandson commented Dec 22, 2023

@armanbilge @benhutchison I think that AnyVal may be (at least given current compiler state of the art) better (see above). for one thing, it allows .value to be just the value field instead of a method, and also benefitting from AnyVal optimizations. Maybe even more significant, .withUnit[U] can be written cleaner, and does not need to invoke the Applier based shim.

It's also a bit easier for me to reason about - the opaque type causes some tricky behaviors and limitations, that AnyVal does not.

@erikerlandson
Copy link
Owner Author

This seems like a very powerful idiom to me:

        inline def toUnit[U]: Quantity[VL, U] =
            import coulomb.conversion.coefficients.*
            inline compiletime.erasedValue[VL] match
                case _: Double =>
                    (coefficientDouble[UL, U] * ql.value.asInstanceOf[Double])
                        .asInstanceOf[VL]
                        .withUnit[U]
                case _ =>
                    compiletime.summonFrom {
                        case conv: UnitConversion[VL, UL, U] =>
                            conv(ql.value).withUnit[U]
                        case _ =>
                            compiletime.error("no!")
                    }

It allows you to have your cake and eat it too: you can really optimize for certain special cases, but still allow the compiler to fall back on typeclasses for anything else.

@benhutchison
Copy link

I'm wary about switching opaque types back to AnyVal.

  • My expectation is that arrays of opaque types of primitives are unboxed. Whereas arrays of AnyVals have to be boxed.. because Scala insists that an AnyVal supports isInstanceOf IIUC.

    IMO performance typically matters when you are at scale. At scale, you typically have alot of data and will likely be using arrays. So dense packing is important.

  • I feel like AnyVals were "ideologically deprecated" in Scala 3 with opaque types the preferred approach. While support may not yet be optimal, my hunch is there will be more future interest & investment in optimizing opaque types than AnyVal.

@erikerlandson
Copy link
Owner Author

I also filed an RFE to inline spire typeclass defs:
typelevel/spire#1309

@erikerlandson
Copy link
Owner Author

@benhutchison my ultimate plan is to define collections that explicitly store raw values under the hood, and so it would not matter if it was opaque or AnyVal:
#530

However, if AnyVal is deprecated, even in the long run, that is another argument. The current situation with opaque type is a bit frustrating, and if there are future improvements planned, I'd be interested to know when.

@benhutchison
Copy link

The current situation with opaque type is a bit frustrating, and if there are future improvements planned, I'd be interested to know when.

Yes, using Scala 3 is a mixture of tantalizing capabilities but also frustrating limitations.

However, I do think they've signalled that opaque types are the "blessed path", although it will likely take time to see improvements. Competing priorities etc.

But looking at the recent 3.4.0 RC1 release notes, it is clear there's alot of work going on..

@erikerlandson
Copy link
Owner Author

@armanbilge I ran into this interesting glitch:
scala/scala3#19523

@erikerlandson
Copy link
Owner Author

@armanbilge on scala/scala3#19523 odersky made a comment:

The mid-term goal should be to get rid of implicit conversions entirely. We should now have better way to do things.

I'm not sure what he was talking about; I thought scala.Conversion was the "better way" - is there now something else?

@erikerlandson
Copy link
Owner Author

erikerlandson commented Feb 2, 2024

conversation on scala discord:

armanbilge — Today at 1:40 PM
yes, for example a common use of implicit conversions in Scala 2 was to add syntax. but in Scala 3 the "better way" is extension

eje — Today at 1:40 PM
extension definitely cool, but that's not really my particular use case
I suppose I could just not support implicit unit conversions, if you want to convert then you have to use q.toUnit[U]  or q.toValue[V]

armanbilge — Today at 1:45 PM
yes, exactly. In scala 3 that is more idiomatic
the fear with implicit conversions is that they obscure what is happening

I don't know how this sits with me, but I do know that supporting scala.Conversion does oddball things to my function signatures, see for example:
scala/scala3#19523 (comment)

And they appear to have no intention of "fixing" this, they don't think scala.Conversion is desirable period.

Eliminating implicit unit/value conversions would even further simplify coulomb's systems.

@erikerlandson
Copy link
Owner Author

A couple observations from morning experiments:

1: I have been Doing It Wrong. I've been calling .withUnit[U] in all my extension methods, but it is (a) not necessary inside of object Quantity and (b) gives better bytecode to not do it :)

        inline def toValue[V](using
            conv: ValueConversion[VL, V]
        ): Quantity[V, UL] =
            conv(ql.value).withUnit[UL] // withUnit call isn't needed here!

        inline def toValue2[V](using
            conv: ValueConversion[VL, V]
        ): Quantity[V, UL] =
            conv(ql.value) // better bytecode!

2: when I started all this, the compiler was not allowing me to use inline with opaque, but now it seems to be OK with that:

    // all these inline seem cool with scala compile now

    inline def apply[V, U](v: V): Quantity[V, U] = v
    inline def apply[U](using a: Applier[U]) = a

    final class Applier[U]:
        inline def apply[V](v: V): Quantity[V, U] = v
    object Applier:
        inline given ctx_Applier[U]: Applier[U] = new Applier[U]

3: withUnit[U] itself can be written so it compiles better like this:

        // use call to `Quantity[V,U](v)` instead of withUnit here
        inline def withUnit[U]: Quantity[V, U] = Quantity[V, U](v)

@erikerlandson
Copy link
Owner Author

update on (2) above: the compiler allows inline with the defs in the opaque type but they do not work properly. possibly this is a compiler regression

@erikerlandson
Copy link
Owner Author

I am leaning toward removing tpow - the more I think about it, the more it feels like a weird nonstandard function, and standard integer powers of integral types like Int or Long are already supported via pow

@erikerlandson
Copy link
Owner Author

Using a combination of:

  • inlined algebra defs
  • compile-time elided ifdefs
  • summonFrom
  • erasedValue
  • summonInline

I can get this:

object foofoo:
    import coulomb.*, coulomb.syntax.*
    import coulomb.policy.standard.given
    import coulomb.policy.algebras.given
    val z = 1d.withUnit[1] + 1d.withUnit[1000]

to compile pretty much into my dream, where it is just raw values and raw constant multiplies, adds, etc:

public final class coulomb.foofoo$ implements java.io.Serializable {
  public static final coulomb.foofoo$ MODULE$;

  public static {};
    Code:
       0: new           #2                  // class coulomb/foofoo$
       3: dup
       4: invokespecial #18                 // Method "<init>":()V
       7: putstatic     #20                 // Field MODULE$:Lcoulomb/foofoo$;
      10: getstatic     #25                 // Field coulomb/quantity$package$.MODULE$:Lcoulomb/quantity$package$;
      13: astore_0
      14: aload_0
      15: astore_1
      16: getstatic     #25                 // Field coulomb/quantity$package$.MODULE$:Lcoulomb/quantity$package$;
      19: astore        4
      21: aload         4
      23: astore        5
      25: dconst_1
      26: dstore_2
      27: getstatic     #25                 // Field coulomb/quantity$package$.MODULE$:Lcoulomb/quantity$package$;
      30: astore        8
      32: aload         8
      34: astore        9
      36: dconst_1
      37: dstore        6
      39: ldc2_w        #26                 // double 1000.0d
      42: dload         6
      44: dmul
      45: dstore        10
      47: dload_2
      48: dload         10
      50: dadd
      51: putstatic     #29                 // Field z:D
      54: return

  public double z();
    Code:
       0: getstatic     #29                 // Field z:D
       3: dreturn
}

@erikerlandson
Copy link
Owner Author

erikerlandson commented Mar 1, 2024

I found a bug with in-lining functions A => B, that the scala people have fixed 🎉
scala/scala3#19724

@erikerlandson
Copy link
Owner Author

@armanbilge @benhutchison

I wanted to update you on my planned changes that are most impactful

  • there are no longer any truncating operations, or corresponding TruncatingXXX typeclasses
  • I will no longer support implicit value conversions. If you want to change a value type, you need to call q.toValue
  • q.toValue now includes both truncating and non-truncating conversions (there is no longer a separate q.tToValue)
  • unit conversions are only defined on fractional types
  • implicit unit conversions are still supported
  • as noted above, the concept of value promotions (like Int + Double => Double) is not longer supported (resulting in many simplifications)

I have been toying with the idea of distinguishing "safe" value conversions, such as Int -> Long, or Float -> Double, etc, from unsafe ones, such as Double -> Float, Long -> Double, etc. This would require something like SafeValueConversion[VF, VT] <: ValueConversion[VF, VT]. My concern is that this would be a bit harder to explain, or reason about. compared to simply having the policy "no implicit value conversions, you have to use q.toValue. It also introduces some additional typeclass complexity.

@benhutchison
Copy link

benhutchison commented Apr 15, 2024

Thanks for the summary @erikerlandson

This will affect my application, but likely just requiring some porting work and not any blockers. I'll post some feedback when i do the port, but that's unlikely to be for a while, ie 1-3 months, due to other commitments.

@erikerlandson
Copy link
Owner Author

This is a problem for the dependent type signatures of *, /, etc
scala/scala3#20434

@erikerlandson
Copy link
Owner Author

@benhutchison @armanbilge I have cut a release candidate for this refactor: v0.9.0-RC1

@benhutchison
Copy link

benhutchison commented Aug 25, 2024

I'll post some feedback when i do the port, but that's unlikely to be for a while, ie 1-3 months, due to other commitments.

Hi Erik, I have been quite distracted as, a few days after writing this back in April, my partner discovered she is pregnant. So current attention focussed on 👶 not 💻.

Now seems possible I won't have opportunity to migrate to latest coulomb until I take some extended months of parental leave, end of year. And even that depends on whether she's a good 👼 or bad 😈 baby..

@erikerlandson
Copy link
Owner Author

@benhutchison congratulations 🎉 No hurry, you can see it's been taking me a while to work through this, and I don't have a baby 😁

@erikerlandson
Copy link
Owner Author

@danielrmeyer if you have a chance I'd be interested in learning how v0.9.0-RC1 works with your ODE solver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants