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

Add small-domain floating-point type MiniFloat to discipline package #4033

Closed
wants to merge 18 commits into from

Conversation

tmccarthy
Copy link
Contributor

This PR adds a new data class to the cats.laws.discipline package: MiniFloat. Similar to MiniInt, MiniFloat is intended to provide a floating-point type whose domain is small enough to have an ExhaustiveCheck instance.

This PR is a re-attempt at #3813, but I have broken out the introduction of MiniFloat into a separate PR.

Note that there is a failing test in the scala-native build. This is due to a bug in scala-native that was fixed in 0.4.1. Accordingly, this PR depends on #4022.

FAQ

How big is the domain of MiniFloat?

As written, MiniFloat has 14 values: NaN, PositiveInfinity, NegativeInfinity, -8.0, -4.0, -2.0, -1.0, -0.5, 0.0, 0.5, 1.0, 2.0, 4.0, and 8.0. This compares to 16 for MiniInt, and 2 for Boolean, which are the other types for which ExhaustiveCheck is defined.

Why not just use a 8- or 16-bit float?

These types have much larger cardinalities (roughly 2⁸ and 2¹⁶ respectively) than MiniFloat that make them too big to be used for exhaustive laws checks.

Why would MiniFloat be useful in the discipline package?

Since #2577, testing function equivalence using Arbitrary has been deprecated with ExhaustiveCheck now the preferred way to test. MiniInt serves this purpose, but I found a use for something like MiniFloat when defining an Eq instance for Fractional. Without something like MiniFloat, we have to fall back on using Arbitrary to define Eq[Fractional[A]] (see here). More broadly, It seems like it would be nice to have another option for tests requiring an ExhaustiveCheck instance. It's also a nice small type that can be used to test the quirky behaviours of very large, very small, and infinite floating-point values.

@@ -24,7 +24,7 @@ class MiniIntSuite extends CatsSuite {

{
implicit val m: CommutativeMonoid[MiniInt] = miniIntMultiplication
checkAll("MiniInt addition", CommutativeMonoidTests[MiniInt].commutativeMonoid)
checkAll("MiniInt multiplication", CommutativeMonoidTests[MiniInt].commutativeMonoid)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated change that fixes a typo in the tests for MiniInt.

Comment on lines +157 to +165
/**
* Note that since `MiniFloat` is used primarily for tests, this `Eq` instance defines `NaN` as equal to itself. This
* differs from the `Order` defined for `Float`.
*/
implicit val catsLawsEqInstancesForMiniFloat: Order[MiniFloat] with Hash[MiniFloat] =
new Order[MiniFloat] with Hash[MiniFloat] {
override def compare(x: MiniFloat, y: MiniFloat): Int = Order[Float].compare(x.toFloat, y.toFloat)
override def hash(x: MiniFloat): Int = Hash[Float].hash(x.toFloat)
}
Copy link
Contributor Author

@tmccarthy tmccarthy Nov 1, 2021

Choose a reason for hiding this comment

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

Because we're using this class in tests it's really only useful if we define Eq such that NaN == NaN. I'm aware that this is slightly unintuitive but otherwise the Eq instance becomes useless.

An alternative approach would be to provide this Eq in such a way that it needs to be manually imported when used. I'd be happy to make that change as needed.

Comment on lines +171 to +175
val miniFloatMax: CommutativeMonoid[MiniFloat] with BoundedSemilattice[MiniFloat] =
new CommutativeMonoid[MiniFloat] with BoundedSemilattice[MiniFloat] {
override def empty: MiniFloat = MiniFloat.NegativeInfinity
override def combine(x: MiniFloat, y: MiniFloat): MiniFloat = if (x > y) x else y
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assuming we define Eq[MiniFloat] as above, I believe this is the main lawful Monoid you can define for MiniFloat. I'd be happy to provide instances for addition and multiplication (with appropriate tests and disclaimers) if requested, but I'm not sure they'd be super useful.

@tmccarthy tmccarthy marked this pull request as ready for review November 9, 2021 23:11
@armanbilge
Copy link
Member

Hi, thanks for the PR! Apologies, just working our way through some of the backlog.

Sorry if I missed it, but I'm confused about one point: do we actually need/use MiniFloat anywhere within Cats itself?

# Conflicts:
#	tests/shared/src/test/scala/cats/tests/MiniFloatSuite.scala
In order to test Invariant[Fractional], we need some way to define an Eq[Invariant[Fractional]] for some fractional type. We do this using Float as the fractional type, but this requires the use of the deprecated cats.laws.discipline.DeprecatedEqInstances#catsLawsEqForFn1. If we instead use MiniFloat, we can take advantage of ExhaustiveCheck[MiniFloat] to use cats.laws.discipline.eq#catsLawsEqForFn1Exhaustive. See typelevel#2577 for a broader explanation of this.
@tmccarthy
Copy link
Contributor Author

Thanks @armanbilge. This PR is obviously a bit old but I think it's in a good place and can still add value.

Sorry if I missed it, but I'm confused about one point: do we actually need/use MiniFloat anywhere within Cats itself?

I've added changes to this PR since you had a look that use MiniFloat to test Invariant[Fractional] (see #4032). This is the where I originally ran into the need for this type.

In order to test whether our Invariant[Fractional] instance is lawful in AlgebraInvariantSuite, we need to be able to define an Eq[Fractional[A]] for some A. The existing code uses Float and derives a Eq[Fractional[Float]]. Unfortunately this is done using Arbitrary[Float] and the deprecated DeprecatedEqInstances.catsLawsEqForFn1 here. I believe this is the only place in the core cats tests that this deprecated instance is still used.

Ideally we would instead use catsLawsEqForFn1Exhaustive, which has been the recommended approach since #2577. However, the key problem is that there is currently no type for which both ExhaustiveCheck and Fractional are defined. Hence, we are in need of a small-domain, floating-point type. One that has the behaviour of Fractional, but is small enough that we can define an ExhaustiveCheck instance. That is what this PR provides.

It's possible there are other ways to an Eq[Fractional[A]] and test the lawfulness of Invariant[Fractional]. If the decision here is that MiniFloat is confusing and doesn't add enough value I'll probably go away and try to find another solution. Because I think we need to get rid of the dependency on DeprecatedEqInstances.catsLawsEqForFn1.

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks! 👍 on avoidng DeprecatedEqInstances, and MiniInt certainly sets precedent.

It is unfortunate we'd have to take on so much non-trivial code essentially to test this one thing (unless there are some other possible uses outside of Cats?).

Out of curiosity could we use some sort of rational extended with infinities/NaN here? At least personally I could reason about that more easily than about floating point arithmetic :)

tmccarthy added a commit to tmccarthy/cats that referenced this pull request May 31, 2022
Currently we use Float when doing laws testing for Invariant[Fractional]. This in turn requires us to generate an Eq[Fractional[Float]], which can only be done using DeprecatedEqInstances.catsLawsEqForFn1. In order to stop using this deprecated instances, in this change we now use MiniInt to do our laws testing for Invariant[Fractional].

Note that this approach is spurious since MiniInt is not a fractional number type. But we can use it to test the lawfulness of the Invariant[Fractional] instance, and there is no "MiniFloat" type that is both small enough to have an ExhaustiveCheck instance and fractional. See typelevel#4033 for a broader discussion of this.
@tmccarthy
Copy link
Contributor Author

unless there are some other possible uses outside of Cats

Personally I think this is a fascinating data type that makes some of the interesting corner-cases of floating-point numbers a lot more obvious. But I'm not sure that's a great reason to put it in the core discipline package, particularly given I've had to trade away some of that behaviour in order to make it useful for testing (I've made NaN == NaN, which isn't the case for Float and Double).

Out of curiosity could we use some sort of rational extended with infinities/NaN here? At least personally I could reason about that more easily than about floating point arithmetic :)

This might be doable but I think it might just be easier to cheat a little bit and use Fractional[MiniInt] to test the laws. It's a little spurious but it never escapes the test suite and it's a lot easier to reason about than this full MiniFloat type.

I have raised #4216 taking this approach which may be a more fruitful way of removing the DeprecatedEqInstances. I'm happy for this PR to be closed in lieu of that one.

@tmccarthy
Copy link
Contributor Author

For posterity's sake, I should note that the Hash consistency test is flakey in Scala native. It seems to work sometimes and not other times, even on the CI? If we ever return to this code in the future that'll need to be fixed before this is merged.

@tmccarthy tmccarthy closed this May 31, 2022
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