Skip to content

Conversation

@zakpatterson
Copy link
Contributor

Fixes (part of) #568.

Without this, posNum and negNum will return None about 1% of the time, as they throw out zeros.
@zakpatterson zakpatterson changed the title Fix make posNum and negNum generators always return values Force posNum and negNum generators always return values Sep 26, 2019
@zakpatterson zakpatterson changed the title Force posNum and negNum generators always return values Force posNum and negNum generators to always return values Sep 26, 2019
@non
Copy link
Contributor

non commented Sep 26, 2019

This looks good. Would you be willing to add one other change? I think unless our Numeric[T] is actually a Fractional[T] we can do something even simpler, i.e.

def posNum[T](implicit num: Numeric[T], c: Choose[T]): Gen[T] = {
  import num._
  num match {
    case _: Fractional[_] =>
      sized(n => c.choose(zero, max(fromInt(n), one)).suchThat(_ != zero))
    case _ =>
      sized(n => c.choose(one, max(fromInt(n), one))
  }
}

The reason this might be better is that if n is one, the fractional types will still be able to generate plenty of non-zero values, but the integral types will be generating zero 50% of the time (resulting in more retries).

@zakpatterson
Copy link
Contributor Author

Yes, that makes sense. I'm playing with it now and I'll push tonight.

@zakpatterson
Copy link
Contributor Author

zakpatterson commented Sep 27, 2019

While I'm staring at this, is there a reason that negNum is not just

  def negNum[T](implicit num: Numeric[T], c: Choose[T]): Gen[T] = 
     posNum[T].map(num.negate(_))

@non
Copy link
Contributor

non commented Sep 27, 2019

@zakpatterson Using negation on negative numbers can be dangerous (since -Int.MinValue == Int.MinValue) but since this number will be positive I think your change makes sense.

This looks good. Thanks! 👍

@zakpatterson
Copy link
Contributor Author

Thanks for the review @non, I learned something.

@non
Copy link
Contributor

non commented Sep 27, 2019

I think the build failure was transient, I'm retrying now.

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.

3 participants