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

Update to algebra 2.7.0 #1115

Merged
merged 11 commits into from
Nov 30, 2021
Merged

Conversation

armanbilge
Copy link
Member

@armanbilge armanbilge commented Nov 28, 2021

This PR re-integrates spire with algebra 2.7.0. The following typeclasses from spire were ported into algebra:

  • GCDRing
  • EuclideanRing
  • Signed / SignedAdditiveCMonoid/ SignedAdditiveAbGroup
  • TruncatedDivision / TruncatedDivisionCRing
  • DivisionRing
  • Field

Now 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, when Signed was ported to algebra it no longer extends Order. Instead it has a member def order: Order. This technique is used to avoid ambiguous implicits (e.g. how FunctorFilter doesn't extend Functor in cats).

As a result of this change, an additional Order constraint had to be propagated across spire where it used to be provided by Signed.

@armanbilge armanbilge changed the title Update/algebra 2.7.0 Update to algebra 2.7.0 Nov 28, 2021
@armanbilge armanbilge linked an issue Nov 28, 2021 that may be closed by this pull request
@armanbilge armanbilge marked this pull request as ready for review November 29, 2021 02:38
Copy link
Collaborator

@cquiroz cquiroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -20,7 +20,7 @@ lazy val scalaCheckVersion = "1.15.4"
lazy val munit = "0.7.29"
lazy val munitDiscipline = "1.0.9"

lazy val algebraVersion = "2.2.3"
lazy val algebraVersion = "2.7.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be rename to catsVersion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, catsVersion = algebraVersion now. But since we don't explicitly depend on cats, I think this is fine for now.

@@ -83,7 +83,7 @@ final class SignedOps[A](a: A)(using s: Signed[A]):
def isSignNonNegative: Boolean = s.isSignNonNegative(a)

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?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be an API change, why?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be implemented in spire?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -117,22 +117,22 @@ final case class Complex[@sp(Float, Double) T](real: T, imag: T)
*
* `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] =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is Order required now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1115 (comment) about Order.

@armanbilge
Copy link
Member Author

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.

@cquiroz
Copy link
Collaborator

cquiroz commented Nov 29, 2021

Yes, we should do an M2

@armanbilge armanbilge merged commit 610e3ed into typelevel:main Nov 30, 2021
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.

Integrate with new algebra typeclasses
3 participants