Conversation
cquiroz
left a comment
cquiroz
left a comment
There was a problem hiding this comment.
Thanks a lot for this. I was expecting the change to be less extensive. Is it so because there are duplicates between algebra and spire?
Why is Order now required in many more places?
| lazy val munitDiscipline = "1.0.9" | ||
|
|
||
| lazy val algebraVersion = "2.2.3" | ||
| lazy val algebraVersion = "2.7.0" |
There was a problem hiding this comment.
Should this be rename to catsVersion?
There was a problem hiding this comment.
Yeah, catsVersion = algebraVersion now. But since we don't explicitly depend on cats, I think this is fine for now.
|
|
||
| final class TruncatedDivisionOps[A](lhs: A)(using ev: TruncatedDivision[A]): | ||
| def toBigIntOpt: Opt[BigInt] = ev.toBigIntOpt(lhs) | ||
| // def toBigIntOpt: Opt[BigInt] = ev.toBigIntOpt(lhs) // TODO port to algebra? |
There was a problem hiding this comment.
This seems to be an API change, why?
There was a problem hiding this comment.
Could it be implemented in spire?
There was a problem hiding this comment.
For some reason it wasn't included in typelevel/algebra#247.
I think it would be good if we could continue to implement it in spire, but I wasn't sure how unless we introduce a new typeclass? Any ideas?
| * `sgn(z) = z / abs(z) = abs(z) / z` | ||
| */ | ||
| def complexSignum(implicit f: Field[T], n: NRoot[T], s: Signed[T]): Complex[T] = | ||
| def complexSignum(implicit f: Field[T], n: NRoot[T], o: Order[T], s: Signed[T]): Complex[T] = |
There was a problem hiding this comment.
Why is Order required now?
|
Because these changes are so extensive, I wonder if we should release a second milestone after this PR merges. I think we should wait to do an RC/final release until #1111 is resolved. |
|
Yes, we should do an M2 |
3 participants
This PR re-integrates spire with algebra 2.7.0. The following typeclasses from spire were ported into algebra:
GCDRingEuclideanRingSigned/SignedAdditiveCMonoid/SignedAdditiveAbGroupTruncatedDivision/TruncatedDivisionCRingDivisionRingFieldNow the spire definitions are removed and instead alias the algebra definitions.
Most of the churn here is due to the changes to
Signed. Previously, in spire,Signed <: Order. However, whenSignedwas ported to algebra it no longer extendsOrder. Instead it has a memberdef order: Order. This technique is used to avoid ambiguous implicits (e.g. howFunctorFilterdoesn't extendFunctorin cats).As a result of this change, an additional
Orderconstraint had to be propagated across spire where it used to be provided bySigned.