search

typelevel / algebra

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

Upgrade to Cats 0.9.0. #200

Closed non closed 7 years ago

non commented 7 years ago

This also upgrades to Scala.js 0.6.14.

johnynek commented 7 years ago

What are you doing up?

non commented 7 years ago

Haha, I just realized that I had said I'd start the release process, so wanted to push it along before bed.

johnynek commented 7 years ago

:+1:

non commented 7 years ago

Great! Assuming things go green I'll merge and release (and then go to sleep).

johnynek commented 7 years ago

Sounds good. Sleep well! On Tue, Jan 17, 2017 at 19:55 Erik Osheim notifications@github.com wrote:

Great! Assuming things go green I'll merge and release (and then go to sleep).

— You are receiving this because you commented.

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

non commented 7 years ago

Property failure:

[info] - [algebra.laws.Rat].field.fromDouble *** FAILED ***
[info]   GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.
[info]    (Discipline.scala:14)
[info]     Falsified after 4 successful property evaluations.
[info]     Location: (Discipline.scala:14)
[info]     Occurred when passed generated values (
[info]       arg0 = 1.9726888167225064E-308
[info]     )
[info]     Label of failing property:
[info]       (2124091560789543/101201126653655309176247673359458653524778324882071059178450679013715169783997673445980191850718562247593538932158405955694904368692896738433506699970369254960758712138283180682233453871046608170619883839236372534281003741712346349309051677824579778170405028256179384776166707307615251266093163754323003131653853870546747392 ?== 998191653946919/50600563326827654588123836679729326762389162441035529589225339506857584891998836722990095925359281123796769466079202977847452184346448369216753349985184627480379356069141590341116726935523304085309941919618186267140501870856173174654525838912289889085202514128089692388083353653807625633046581877161501565826926935273373696) failed
johnynek commented 7 years ago

Is that Scala.js or Scala? On Tue, Jan 17, 2017 at 20:01 Erik Osheim notifications@github.com wrote:

Property failure:

[info] - [algebra.laws.Rat].field.fromDouble FAILED [info] GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation. [info] (Discipline.scala:14) [info] Falsified after 4 successful property evaluations. [info] Location: (Discipline.scala:14) [info] Occurred when passed generated values ( [info] arg0 = 1.9726888167225064E-308 [info] ) [info] Label of failing property: [info] (2124091560789543/101201126653655309176247673359458653524778324882071059178450679013715169783997673445980191850718562247593538932158405955694904368692896738433506699970369254960758712138283180682233453871046608170619883839236372534281003741712346349309051677824579778170405028256179384776166707307615251266093163754323003131653853870546747392 ?== 998191653946919/50600563326827654588123836679729326762389162441035529589225339506857584891998836722990095925359281123796769466079202977847452184346448369216753349985184627480379356069141590341116726935523304085309941919618186267140501870856173174654525838912289889085202514128089692388083353653807625633046581877161501565826926935273373696) failed

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/typelevel/algebra/pull/200#issuecomment-273391550, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEJdn2B8A-Hra0Ly-q6gXJ-svZfkdVKks5rTarGgaJpZM4LmgbJ .

non commented 7 years ago

It's on the JVM, we skip that test in Scala.js.

johnynek commented 7 years ago

They fixed that bug I thought? (In scala.js) On Tue, Jan 17, 2017 at 20:04 Erik Osheim notifications@github.com wrote:

It's on the JVM, we skip that test in Scala.js.

— You are receiving this because you commented.

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

non commented 7 years ago

It seems like this is a legit bug:

val x = Field[Rat].fromDouble(1.9726888167225064E-308)
BigDecimal(x.num) / BigDecimal(x.den) // 2.098881337614853913158104485839156E-308

Whereas the "expected" part of the test produces 1.972688816722506443225976254345907E-308 as a result (when evaluated the same way).

non commented 7 years ago

/cc @tixxit

I'm fine with opening an issue for this, merging this PR, and releasing. But if it's easy to fix it would be nice to do so first.

non commented 7 years ago

I think the bug here is in handling sub-normal floating point values. e-307 and above seem fine, but e-308 and below (towards zero) have problems.

tixxit commented 7 years ago

Hrmmm - I thought I had handled the case where the implicit 1 drops when exp bits are 0, but maybe not? I'll take a look and see if I can fix it up quickly.

tixxit commented 7 years ago

Alright - I got a fix incoming. I'll push to this branch.

codecov-io commented 7 years ago

Current coverage is 73.84% (diff: 100%)

Merging #200 into master will increase coverage by 0.19%

@@             master       #200   diff @@
==========================================
  Files            37         37          
  Lines           774        776     +2   
  Methods         713        714     +1   
  Messages          0          0          
  Branches         57         58     +1   
==========================================
+ Hits            570        573     +3   
+ Misses          204        203     -1   
  Partials          0          0

Powered by Codecov. Last update e9c5b22...cb6e950

johnynek commented 7 years ago

👍

I guess we can merge? Since Three of us seem in agreement?