Algebra in core #254

erikerlandson · 2022-03-06T18:05:29Z

port operations to algebraic typeclasses defined in algebra.ring

erikerlandson · 2022-03-06T18:07:48Z

@armanbilge my port for Div isn't working, I'm a bit confused about how to import instances for TruncatedDivision, or possibly about how to cleanly distinguish types supporting MultiplicativeGroup[V] from types supporting TruncatedDivision[V]

armanbilge · 2022-03-06T18:18:39Z

It looks like the compiler is failing because it cannot find TruncatedDivision instances for Int and Long. The only lawful instance for TruncatedDivision in algebra is for BigInt.
https://github.com/typelevel/cats/blob/41b99ed87ab56b32c7b793530aaeeaf5957a236e/algebra-core/src/main/scala/algebra/instances/bigInt.scala#L59

armanbilge · 2022-03-06T18:24:41Z

I guess we can add so-called "alley-cat" instances, that are kind of ok but not really lawful. Some discussion in typelevel/algebra#247 (comment).

erikerlandson · 2022-03-06T18:37:49Z

I can probably punt Int and Long back to scala.math.Integral, for now. alley-cats instances seems like a reasonable thing to add

armanbilge · 2022-03-06T18:42:37Z

I think the bigger issue here is / in MultiplicativeGroup and "/" in TruncatedDivision are not really the same operation. I'm not sure I feel comfortable representing those with the same symbol/syntax.

Let me check what spire does.

armanbilge · 2022-03-06T18:48:03Z

Yeah, spire doesn't use / for TruncatedDivision:
https://github.com/typelevel/spire/blob/610e3edb8d4e6ef6b1f7b99a568df33d1237eb1a/tests/shared/src/test/scala/spire/SyntaxScalaCheckSuite.scala#L161

So personally I'm 👎 on using / for both. It is confusing and surprising.

erikerlandson · 2022-03-06T19:07:40Z

It could be cleaner to just support quot instead. I'm a bit confused at why algebra calls it tquot instead of just quot like Numeric does. using quot for integral quotient seems cleaner than tquot.

In coulomb, there is a parallel distinction where unit conversions are concerned. Possibly there is some analogy relating to unit conversions involving integral quotients. Unsure what such a policy might be, possibly:

implicit conversions involving integral quotients is not supported
maybe even a separate toUnitQuot method, and a UnitConversionQuot typeclass
in such a system, allowTruncation would no longer be required

Having grown up in a world where 5 / 2 => 2 was the de facto semantic for / on integers, part of me thinks this is needless, on the other hand I clearly also consider it a distinct policy, or I would not have created allowTruncation in the first place, and it seems like quot is the de facto semantic in the typelevel ecosystem.

My inclination would be to use quot instead of tquot, do you have an opinion on this?

armanbilge · 2022-03-06T19:17:09Z

It's because algebra distinguishes between tquot (which truncates) and fquot (which floors). What does ~~Numeric#quot~~ Integral#quot do?

 * We define functions tmod and tquot such that:
 * q = tquot(x, y) and r = tmod(x, y) obey rule (4t),
 * (which truncates effectively towards zero)
 * and functions fmod and fquot such that:
 * q = fquot(x, y) and r = fmod(x, y) obey rule (4f)
 * (which floors the quotient and effectively rounds towards negative infinity).

https://github.com/typelevel/cats/blob/41b99ed87ab56b32c7b793530aaeeaf5957a236e/algebra-core/src/main/scala/algebra/ring/TruncatedDivision.scala#L22

I think it's fine to go with quot as long as its well-specified. tquot and fquot starts to get arcane :)

erikerlandson · 2022-03-06T21:48:41Z

NRoot seems to be defined for all of the integral types, in essentially the same truncating way I defined CanPow. That seems un-aligned with the policy of requiring TruncatingDivision for tquot. You require a special typeclass and operator for truncating division, but lump both truncating and non-truncating roots together.

armanbilge · 2022-03-06T22:08:38Z

Good observation :)

NRoot may need some re-thinking. It is older than TruncatedDivision (typelevel/spire#603 in 2016). NRoot was typelevel/spire#6 in 2012! I also can't find any laws for it, unlike TruncatedDivision. See also:

Make NRoot more formal typelevel/spire#741

erikerlandson · 2022-03-07T22:46:11Z

@cquiroz @armanbilge

There are two main changes proposed on this PR

coulomb.ops.standard.given are now written using algebra typeclasses. Typelevel algebra is now a dependency of coulomb-core.
All operations involving integral truncation are now "explicit"

"explicit truncation" takes two main forms.

Truncating conversions never happen implicitly, or automatically.
To invoke truncating operations, there are four new special truncating methods: tquot, tpow, tToUnit and tToValue

The unit tests provide a feel for what all this looks like in the code.

core/src/main/scala/coulomb/ops/standard/tquot.scala

armanbilge · 2022-03-07T23:09:13Z

core/src/main/scala/coulomb/conversion/standard/standard.scala

+inline given ctx_TVC_Long[VF](using num: Fractional[VF]): TruncatingValueConversion[VF, Long] =
+    new TruncatingValueConversion[VF, Long]:
        def apply(v: VF): Long = num.toLong(v)


This is a bikeshed for another issue/PR, but I'm concerned that inline combined with new ... is sub-optimal and can lead to a proliferation of anonymous classes in the jar. It depends on whether the compiler will automatically recognize this is a SAM and use invokedynamic instead. It might be better to implement this as a lambda e.g. v => num.toLong(v) to encourage this optimization.

I'm less familiar with Scala.js internals but this could be especially bad for JavaScript actually.

See typelevel/cats#3871 for a discussion of this topic.

Actually, it's not obvious to me why inline is needed here?

Do not assume I'm using these only when they are needed 😁

I'm still working out my intuitions about when inline and transparent inline are necessary. Situations where a typeclass involved dependent typing seem unambiguous - they need transparent inline. Others are less obvious (to me).

Some places I am definitely using inline for efficiency gains, but other places it may be a bad trade

At any rate, if inline is problematic, I'm happy to clean all that up where it is not needed. Having some decent unit test coverage should make this a bit easier now. If something introduces a compile error, it ought to show up pretty fast.

Yes, I would use inline where needed only for its compile-time semantics.

For runtime performance, both the JVM and the Scala.js linker backend do a fantastic job of inlining themselves.

We can always revisit inline opportunities in the future with a benchmark suite.

We have had bad experiences with macro generated classes in the past. When defining boopickle generated serialized it was very easy to endup generating thousands of instances that would bloat the resulting js size. It wasn't really noticeable on the jvm thus it was quite hard to spot

overall I expect scala-3 coulomb to be better in this regard. I am using metaprogramming in multiple places where I was using intermediate typeclasses, and so these intermediate typeclasses should no longer appear in byte code. However, some of this may be offset by use of transparent inline, so it will definitely be helpful to remove any unimportant usages of inline

armanbilge · 2022-03-07T23:37:30Z

core/src/main/scala/coulomb/quantity.scala

+    transparent inline def tquot[VR, UR](qr: Quantity[VR, UR])(using tq: TQuot[VL, UL, VR, UR]): Quantity[tq.VO, tq.UO] =
+        tq(ql.value, qr.value).withUnit[tq.UO]
+
    transparent inline def pow[P](using pow: Pow[VL, UL, P]): Quantity[pow.VO, pow.UO] =
        pow(ql.value).withUnit[pow.UO]

+    transparent inline def tpow[P](using tp: TPow[VL, UL, P]): Quantity[tp.VO, tp.UO] =
+        tp(ql.value).withUnit[tp.UO]


Consider adding the infix modifier?

I might. In scala 3 it's almost as easy to just use backquotes if you really want to go infix

q1 `tquot` q2

but will consider

Yeah, I learned that one today :)

armanbilge · 2022-03-07T23:52:41Z

👍 thank you so much for pursuing this. I think an algebra dependency in core definitely pays off.

The unit tests look really, really good :) the implementation complexity feels a bit overwhelming tbh, but it's well hidden from the user experience AFAICT.

cquiroz · 2022-03-08T00:44:42Z

It looks great. I think having to explicity say when you truncate is an overall win

erikerlandson · 2022-03-08T01:17:57Z

I'm glad you like it 🎉 It will be a couple days before I can get back to this. I'm going to stew on it and merge it when I can get back to it and refactor the algebra bits a little.

erikerlandson · 2022-03-12T15:25:40Z

refactorings relating to the use of algebra are complete, so I'm going to merge this back into main scala3 dev

erikerlandson added 4 commits March 6, 2022 10:37

port Add to tl algebra

3d2bede

port Sub to tl algebra

db7cf20

port Mul to tl algebra

2296aff

Div with TruncatedDivision is failing

fe55800

erikerlandson added 5 commits March 7, 2022 09:52

/ and tquot

e78652a

neg

da94832

pow and tpow

746798b

remove algebra.scala

79993dd

explicit truncating conversions

ec6d576

armanbilge reviewed Mar 7, 2022

View reviewed changes

core/src/main/scala/coulomb/ops/standard/tquot.scala Outdated Show resolved Hide resolved

armanbilge reviewed Mar 7, 2022

View reviewed changes

erikerlandson added 4 commits March 10, 2022 16:23

populate some coulomb.ops.algebra instances

540299e

refactor use of algebras

b30eae2

scala.math.Ordering -> cats.kernel.Order

a72a408

refactor si, si.prefixes, mks, mksa to use export

1ee822f

erikerlandson added 4 commits March 11, 2022 16:36

tweak directories

c39fc91

refactor algebra to use export

15b3dd5

factor value-specific optimizations to separate import

3b56b9d

unit test optimized operations

6d2c43c

erikerlandson merged commit 201871f into scala3 Mar 12, 2022

erikerlandson deleted the algebra-in-core branch March 28, 2022 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Algebra in core #254

Algebra in core #254

erikerlandson commented Mar 6, 2022

erikerlandson commented Mar 6, 2022

armanbilge commented Mar 6, 2022

armanbilge commented Mar 6, 2022

erikerlandson commented Mar 6, 2022

armanbilge commented Mar 6, 2022

armanbilge commented Mar 6, 2022

erikerlandson commented Mar 6, 2022

armanbilge commented Mar 6, 2022 •

edited

Loading

erikerlandson commented Mar 6, 2022

armanbilge commented Mar 6, 2022

erikerlandson commented Mar 7, 2022

armanbilge Mar 7, 2022 •

edited

Loading

armanbilge Mar 7, 2022

erikerlandson Mar 7, 2022

armanbilge Mar 7, 2022

cquiroz Mar 8, 2022

erikerlandson Mar 11, 2022

armanbilge Mar 7, 2022

erikerlandson Mar 7, 2022

armanbilge Mar 7, 2022

armanbilge commented Mar 7, 2022

cquiroz commented Mar 8, 2022

erikerlandson commented Mar 8, 2022

erikerlandson commented Mar 12, 2022

Algebra in core #254

Algebra in core #254

Conversation

erikerlandson commented Mar 6, 2022

erikerlandson commented Mar 6, 2022

armanbilge commented Mar 6, 2022

armanbilge commented Mar 6, 2022

erikerlandson commented Mar 6, 2022

armanbilge commented Mar 6, 2022

armanbilge commented Mar 6, 2022

erikerlandson commented Mar 6, 2022

armanbilge commented Mar 6, 2022 • edited Loading

erikerlandson commented Mar 6, 2022

armanbilge commented Mar 6, 2022

erikerlandson commented Mar 7, 2022

armanbilge Mar 7, 2022 • 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

Choose a reason for hiding this comment

armanbilge commented Mar 7, 2022

cquiroz commented Mar 8, 2022

erikerlandson commented Mar 8, 2022

erikerlandson commented Mar 12, 2022

armanbilge commented Mar 6, 2022 •

edited

Loading

armanbilge Mar 7, 2022 •

edited

Loading