typelevel / algebra

Experimental project to lay out basic algebra type classes
https://typelevel.org/algebra/
Other
378 stars 69 forks source link

Add JMH-based benchmark project with example benchmarks #194

Closed sritchie closed 7 years ago

sritchie commented 7 years ago

Closes #31. This PR also

codecov-io commented 7 years ago

Current coverage is 73.99% (diff: 100%)

Merging #194 into master will not change coverage

@@             master       #194   diff @@
==========================================
  Files            36         36          
  Lines           769        769          
  Methods         708        708          
  Messages          0          0          
  Branches         61         61          
==========================================
  Hits            569        569          
  Misses          200        200          
  Partials          0          0          

Powered by Codecov. Last update 06f311f...d1e0307

johnynek commented 7 years ago

How can we have broken mima compat with 0.6.0 if mima is turned on? If we broke compatibility we need to fix it. Or make a new release with mima on.

We are going to get burned if we have this at the bottom of algebird and Spire but break compatibility all the time.

sritchie commented 7 years ago

None of the projects were specifying their youngest compatible versions. I added those and boom, failure. On Sat, Dec 3, 2016 at 10:33 AM P. Oscar Boykin notifications@github.com wrote:

How can we have broken mima compat with 0.6.0 if mima is turned on? If we broke compatibility we need to fix it. Or make a new release with mima on.

We are going to get burned if we have this at the bottom of algebird and Spire but break compatibility all the time.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/typelevel/algebra/pull/194#issuecomment-264653201, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEQA8kUT_cy8uRClvy_Mt-W82YHSXDTks5rEafZgaJpZM4LDWxA .

non commented 7 years ago

Oh jeez. Can you add a comment with the binary incompatibilities?

sritchie commented 7 years ago

yeah, one second...

sritchie commented 7 years ago
[info] core: found 1 potential binary incompatibilities while checking against org.typelevel:algebra_2.12:0.6.0
[error]  * method this()Unit in class algebra.instances.BigDecimalAlgebra does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("algebra.instances.BigDecimalAlgebra.this")
[info] Done updating.
[info] benchmark: previous-artifact not set, not analyzing binary compatibility
[info] laws: found 0 potential binary incompatibilities while checking against org.typelevel:algebra-laws_2.12:0.6.0
[info] laws: found 0 potential binary incompatibilities while checking against org.typelevel:algebra-laws_2.12:0.6.0
[info] core: found 1 potential binary incompatibilities while checking against org.typelevel:algebra_2.12:0.6.0
[error]  * method this()Unit in class algebra.instances.BigDecimalAlgebra does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("algebra.instances.BigDecimalAlgebra.this")
johnynek commented 7 years ago

Ahh, this was Thomas's change to accept a parameter to the constructor. We can fix that by adding a default constructor with no params. Problem solved.

def this() = this(BigDecimal... On Sat, Dec 3, 2016 at 10:28 Sam Ritchie notifications@github.com wrote:

[info] core: found 1 potential binary incompatibilities while checking against org.typelevel:algebra_2.12:0.6.0 [error] method this()Unit in class algebra.instances.BigDecimalAlgebra does not have a correspondent in current version [error] filter with: ProblemFilters.excludeDirectMissingMethodProblem [info] Done updating. [info] benchmark: previous-artifact not set, not analyzing binary compatibility [info] laws: found 0 potential binary incompatibilities while checking against org.typelevel:algebra-laws_2.12:0.6.0 [info] laws: found 0 potential binary incompatibilities while checking against org.typelevel:algebra-laws_2.12:0.6.0 [info] core: found 1 potential binary incompatibilities while checking against org.typelevel:algebra_2.12:0.6.0 [error] method this()Unit in class algebra.instances.BigDecimalAlgebra does not have a correspondent in current version [error] filter with: ProblemFilters.excludeDirectMissingMethodProblem

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/typelevel/algebra/pull/194#issuecomment-264663436, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEJdmZ07IbUnRhT5J9RVh3wjP5WQWP8ks5rEdDjgaJpZM4LDWxA .

non commented 7 years ago

👍 Thanks for catching this @sritchie

sritchie commented 7 years ago

@johnynek @non that addition worked great! I changed it and re-enabled mima. let's see how the build goes.

non commented 7 years ago

If that works I can try to do a rush 0.6.1 release to preserve 0.5.x compatibility.

sritchie commented 7 years ago

Ah, is this 0.5 compatible too? If so feel free to change the minimum version in this PR! I've gotta pull myself away for the weekend. On Sat, Dec 3, 2016 at 1:53 PM Erik Osheim notifications@github.com wrote:

If that works I can try to do a rush 0.6.1 release to preserve 0.5.x compatibility.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/typelevel/algebra/pull/194#issuecomment-264664803, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEQA01qAZiDpOCQ62W_chg-a41rPsi3ks5rEda8gaJpZM4LDWxA .

non commented 7 years ago

No, sorry, I got confused on which version we were on. Ignore me!

sritchie commented 7 years ago

Hmmm, looks like we have another legit binary compatibility issue. This commit is the culprit: https://github.com/typelevel/algebra/commit/24fe85a71d58056d07c954f9a1a900159a053ea8

it renames Ring.fromDouble to defaultFromDouble.

[error]  * method defaultFromDouble(Double,algebra.ring.Ring,algebra.ring.MultiplicativeGroup)java.lang.Object in trait algebra.ring.RingFunctions is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("algebra.ring.RingFunctions.defaultFromDouble")
[info] core: found 1 potential binary incompatibilities while checking against org.typelevel:algebra_2.11:0.6.0
[error]  * method defaultFromDouble(Double,algebra.ring.Ring,algebra.ring.MultiplicativeGroup)java.lang.Object in trait algebra.ring.RingFunctions is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("algebra.ring.RingFunctions.defaultFromDouble")

@non @johnynek, do one of you want to fix this in a separate PR, or commit to this PR? Might be better to use this one to leverage the mima check.

johnynek commented 7 years ago

Maybe it is best to make a PR to turn Mima back on and fix the issues (or quickly make a 0.7.0 of algebra).

One idea to fix issues would be to add methods to RingFunctions[R] using an implicit class. Kind of round about, but safe for binary compatibility and makes source compatibility closer to binary compatibility.

sritchie commented 7 years ago

:boom: done @johnynek, and thanks for the suggested fix!

thomas-stripe commented 7 years ago

👍

sritchie commented 7 years ago

@thomas-stripe, can I trouble you for a merge?