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

Fix #396 by avoiding .toDouble in Modulo Validate #398

Merged
merged 7 commits into from
Jan 2, 2018

Conversation

howyp
Copy link
Contributor

@howyp howyp commented Jan 1, 2018

Fixes #396.

This switches the Validate instances for Modulo to use Integral rather than Numeric. I've also kept the support for floating point types by having specific instances for them, though I'd vote for dropping this support.

I couldn't find a way to create a witness-style singleton for Byte and Short literals - hence they are not tested.

The change is almost binary-compatible. I've had to add an exclude for ReversedMissingMethodProblem - is that ok? I see there are other exclusions of this type already.

@codecov
Copy link

codecov bot commented Jan 1, 2018

Codecov Report

Merging #398 into master will decrease coverage by 0.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #398      +/-   ##
==========================================
- Coverage   96.63%   96.16%   -0.48%     
==========================================
  Files          43       43              
  Lines         505      521      +16     
  Branches       10       11       +1     
==========================================
+ Hits          488      501      +13     
- Misses         17       20       +3
Impacted Files Coverage Δ
...ed/src/main/scala/eu/timepit/refined/numeric.scala 91.17% <0%> (-8.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3fc8b9...3d60e0a. Read the comment docs.

@fthomas
Copy link
Owner

fthomas commented Jan 1, 2018

LGTM, thanks @howyp!

I think the MiMa exclusions are fine, since the trait we are adding methods to has visibility private[refined].

Any specific reasons for dropping the Float and Double instances?

i: Integral[T]
): Validate.Plain[T, Modulo[N, O]] =
Validate.fromPredicate(
t ⇒ i.rem(t, i.fromInt(tn())) == to(),
Copy link
Owner

Choose a reason for hiding this comment

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

to() should be i.fromInt(to()) so that we don't compare unrelated types.

@howyp
Copy link
Contributor Author

howyp commented Jan 1, 2018

I know the JVM supports modulus on floating-point values, so in some sense it's a meaningful thing for refined to support. But for evaluating even/oddness it gives unexpected results once you get into large numbers:

scala> refineV[Even](Double.MaxValue)
res8: Either[String,eu.timepit.refined.api.Refined[Double,eu.timepit.refined.numeric.Even]] = Right(1.7976931348623157E308)

scala> refineV[Even](Double.MaxValue - 1)
res9: Either[String,eu.timepit.refined.api.Refined[Double,eu.timepit.refined.numeric.Even]] = Right(1.7976931348623157E308)

@fthomas
Copy link
Owner

fthomas commented Jan 1, 2018

That seems correct, given

scala> Double.MaxValue == (Double.MaxValue - 1)
res8: Boolean = true

As long as the instances are consistent with _ % 2.0 == 0.0, they are fine with me.

@howyp
Copy link
Contributor Author

howyp commented Jan 2, 2018

Maybe I just have a dislike of the weirdness of operations on floating-point numbers :-( Let me know if you'd rather move this discussion to Gitter/another issue, the PR is GTG from my point of view.

I think if you're using the Modulo or maybe the Divisible predicate, then being consistent with the underlying operations could make sense. But I found it very unexpected that all floating-points over a certain size are judged to be Even. Maybe I expect the concept of 'even'/'odd' to refer to integer numbers.

If you're keen on keeping the support for floating-points with Even/Odd, then I think the definition of Odd might need revisiting. It's currently type Odd = Not[Even]. For integer types this is clearly correct, but for floating points then all numbers in-between even integers will be judged Odd, for instance 2.0 is even, but 1.9, 2.1 are odd. For instance:

refineV[Even](2.0)
res0: Either[String,eu.timepit.refined.api.Refined[Double,eu.timepit.refined.numeric.Even]] = Right(2.0)

scala> refineV[Even](2.1)
res1: Either[String,eu.timepit.refined.api.Refined[Double,eu.timepit.refined.numeric.Even]] = Left(Predicate failed: (2.1 % 2 == 0).)

scala> refineV[Even](1.9)
res2: Either[String,eu.timepit.refined.api.Refined[Double,eu.timepit.refined.numeric.Even]] = Left(Predicate failed: (1.9 % 2 == 0).)

scala> refineV[Odd](1.0)
res3: Either[String,eu.timepit.refined.api.Refined[Double,eu.timepit.refined.numeric.Odd]] = Right(1.0)

scala> refineV[Odd](1.1)
res4: Either[String,eu.timepit.refined.api.Refined[Double,eu.timepit.refined.numeric.Odd]] = Right(1.1)

scala> refineV[Odd](0.9)
res5: Either[String,eu.timepit.refined.api.Refined[Double,eu.timepit.refined.numeric.Odd]] = Right(0.9)

I found that behaviour quite surprising. Would type Odd = Modulo[_2, _1] be a better definition?

@fthomas
Copy link
Owner

fthomas commented Jan 2, 2018

You've convinced me. Even and odd only have a precise definition for integers and the choice refined makes for floating point types will always be arbitrary because there is no common definition what odd and even means for floats. And since we have defined Even and Odd via Modulo the only way to not support Even / Odd for floats is to drop the Modulo instances for floats.

👍 for dropping those instances.

@howyp
Copy link
Contributor Author

howyp commented Jan 2, 2018

Excellent! The instances have gone. Let me know if anything else is needed on this

@fthomas fthomas merged commit cb4747c into fthomas:master Jan 2, 2018
@howyp howyp deleted the long-modulo branch January 2, 2018 21:33
@fthomas
Copy link
Owner

fthomas commented Jan 2, 2018

Everything was fine. Thanks again for your patience. :-)

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.

2 participants