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

Invariant[Fractional] and MiniFloat #3813

Closed
wants to merge 16 commits into from

Conversation

tmccarthy
Copy link
Contributor

Fixes #3794

This PR adds Invariant[Fractional]. In order to test this instance, I have provided a new class MiniFloat, which is the floating-point analogue to MiniInt. Its range is small enough that an ExhaustiveCheck instance can be defined.

I'm conscious that a class like MiniFloat is a more significant addition than might be strictly necessary to add an instance like Invariant[Fractional], but I'm hoping that it's judged to be sufficiently useful to be included in the discipline library.

I'll make more explanatory comments inline on the PR.

Note that the build is failing due to a bug in the Scala Native implementation of isFinite. I'll try to find a solution to ignore this test.

@@ -67,3 +69,7 @@ trait AllInstancesBinCompat5 extends SortedSetInstancesBinCompat0
trait AllInstancesBinCompat6 extends SortedSetInstancesBinCompat1 with SortedMapInstancesBinCompat2

trait AllInstancesBinCompat7 extends SeqInstances

trait AllInstancesBinCompat8 extends InvariantInstances
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I missed adding the InvariantInstances to the 2.12 traits in #3773, so the cats.instances.all._ import didn't include them in 2.12. This is fixed here.

* to its small domain. It is only approximately commutative and associative under addition and multiplication, due to
* floating-point errors, overflows, and the behaviour of `NaN`.
*
* Note that unlike `Float`, `MiniFloat` does not support the representation of negative zero (`-0f`).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I've made the decision not to reproduce Float's behaviour around -0f. This lets us ignore the complications around having two distinct values for 0 that are considered equal by any Eq definitions we are interested in using.

Comment on lines +34 to +39
override def equals(other: Any): Boolean = other match {
case that: MiniFloat => this.toFloat == that.toFloat
case _ => false
}

override def hashCode: Int = java.lang.Float.hashCode(toFloat)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This replicates the universal equality behaviour of Float, which is not consistent with the decision I've made below to define Eq[MiniFloat] as considering NaN as equal to itself. I will discuss this below.

Comment on lines +54 to +60
private[MiniFloat] val base = 2

private val minSignificand = -2
private val maxSignificand = 2

private val minExponent = -1
private val maxExponent = 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These define the range of MiniFloat. The finite values currently are:

  • -8.0
  • -4.0
  • -2.0
  • -1.0
  • -0.5
  • 0.0
  • 0.5
  • 1.0
  • 2.0
  • 4.0
  • 8.0

Comment on lines +114 to +122
private val floatExponentStartBit: Int = 23
private val floatExponentLength: Int = 8
private val floatExponentBias: Int = 127
private val floatExponentMask: Int = ((1 << floatExponentLength) - 1) << floatExponentStartBit

// This does the same thing as java.lang.Math.getExponent, but that method is not available in scalaJS so we have to
// do the same thing here.
private def getExponent(float: Float): Int =
((floatExponentMask & java.lang.Float.floatToIntBits(float)) >> floatExponentStartBit) - floatExponentBias
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I've had to write a dedicated function to extract the exponent from a Float since java.lang.Math.getExponent is not currently defined in ScalaJS. I'll probably look to add this over there soon.

}

protected def versionSpecificNumericEq[A: Eq: ExhaustiveCheck]: Eq[Numeric[A]] = Eq.allEqual
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no version-specific components of Numeric in 2.12, as opposed to 2.13 which added parseString.

Comment on lines -29 to -50
implicit protected def eqNumeric[A: Eq: ExhaustiveCheck]: Eq[Numeric[A]] = Eq.by { numeric =>
// This allows us to catch the case where the fromInt overflows. We use the None to compare two Numeric instances,
// verifying that when fromInt throws for one, it throws for the other.
val fromMiniInt: MiniInt => Option[A] =
miniInt =>
try Some(numeric.fromInt(miniInt.toInt))
catch {
case _: IllegalArgumentException => None // MiniInt overflow
}

(
numeric.compare _,
numeric.plus _,
numeric.minus _,
numeric.times _,
numeric.negate _,
fromMiniInt,
numeric.toInt _,
numeric.toLong _,
numeric.toFloat _,
numeric.toDouble _
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this code for defining an Eq[Numeric] is common between 2.12 and 2.13, so I've refactored to move the common stuff into the common file. Only the version-specific behaviour sits in ScalaVersionSpecific.scala via the versionSpecificNumericEq below.

)
}

Eq.and(versionSpecificNumericEq, versionAgnosticNumericEq)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to feedback about this approach of breaking the version-specific equality checks out into a dedicated Eq in ScalaVersionSpecific.scala.

import org.scalacheck.Prop._
import org.scalacheck.{Arbitrary, Gen}

class MiniFloatSuite extends CatsSuite {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a lot of tests in here, which I hope is OK. I wanted to be thorough when checking the corner-case floating point behaviours.

}
}

testSpecialNumberExpectations(MiniFloat.NaN, expectIsNaN = true, expectIsFinite = false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test fails in Scala Native due to scala-native/scala-native#2178.

@tmccarthy tmccarthy marked this pull request as ready for review February 25, 2021 03:28
@johnynek
Copy link
Contributor

johnynek commented Mar 5, 2021

I'd personally like to separate the PR. One for the Invariant[Fractional] and another for MiniFloat.

I'm more skeptical to MiniFloat. I think something like Float16:

https://github.com/stripe/agate/blob/master/core/src/main/scala/com/stripe/agate/tensor/Float16.scala

or BFloat16:

https://github.com/stripe/agate/blob/master/core/src/main/scala/com/stripe/agate/tensor/BFloat16.scala

would be more useful.

It is small enough to enumerate still, but also potentially useful for low precision applications.

@tmccarthy
Copy link
Contributor Author

I'd personally like to separate the PR. One for the Invariant[Fractional] and another for MiniFloat.

I probably could have been clearer in the issue description, but these are bundled together in this PR because MiniFloat is needed for the tests.

In order to define an Eq[Fractional[A]], we can either:

No type currently exists for which you can define both Fractional and ExhaustiveCheck, hence the need for a type like MiniFloat.

If the scope of this PR is too big I'm happy to use the deprecated catsLawsEqForFn1 using Arbitrary[Float] in order to add Invariant[Fractional], but my preference was to avoid the use of the deprecated function.

I'm more skeptical to MiniFloat. I think something like Float16 or BFloat16 would be more useful.
It is small enough to enumerate still, but also potentially useful for low precision applications.

I might be missing something, but a Float16 type has a cardinality of roughly 2¹⁶ = 65,536 (a bit less if you equate all NaNs). This seems much too big for ExhaustiveCheck. Certainly it's orders of magnitude more than for the other types for which cats provides a definition (2 for Boolean, 16 for MiniInt, 14 for MiniFloat here).

Also, my understanding was that MiniFloat (and MiniInt, which inspired it) are intended solely for use in laws testing using ExhaustiveCheck. I'm not sure either of them needs to have any utility for "low precision applications". Perhaps cats (or spire) might like to define a Float16 type separately, but that would presumably in the main library and not in discipline like MiniFloat.

Base automatically changed from master to main March 20, 2021 10:41
@tmccarthy
Copy link
Contributor Author

I'll break this into two PRs.

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.

Add Invariant[Fractional]
2 participants