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

[WIP] Revamp instances of Arbitrary for Numeric #106

Merged
merged 1 commit into from Dec 18, 2015

Conversation

jedesah
Copy link
Contributor

@jedesah jedesah commented Dec 14, 2015

  • Remove hardcoded Int
  • Add instances for shapeless.Nat

- Remove hardcoded Int
- Add instances for shapeless.Nat
@jedesah
Copy link
Contributor Author

jedesah commented Dec 14, 2015

There is still a few things to add. In particular an extra property test, as it stands this will probably decrease coverage, but wanted to open the PR sooner rather than later in order to get feedback and avoid extra work.

@fthomas
Copy link
Owner

fthomas commented Dec 14, 2015

Thanks for the PR, @jedesah. I'll take a deeper look at it later today.

checkArbitraryRefType[Refined, Long, Greater[Nat._10]]

property("Greater range") =
forAll { (min: Refined[Long, Greater[Nat._10]], max: Refined[Long, Greater[Nat._10]]) =>
Copy link
Owner

Choose a reason for hiding this comment

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

If min > max greaterArbitraryRange[Refined, Long, Nat._10](min, max) probably doesn't produce any values which seems to be the cause of the failed Travis build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, that seems like a reasonable hypothesis. Can probably add a precondition on the outer property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried that. The filter is still triggering scalacheck to abandon. I think there is a way to increase the ratio of discarded examples without triggering the property to abandon the "proof". Or is there a better way to encode information in the types?

Copy link
Owner

Choose a reason for hiding this comment

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

You could do a runtime check which of min and max is the smaller resp. greater value and then pass the smaller as first arg and the greater as second arg to greaterArbitraryRange.

@fthomas
Copy link
Owner

fthomas commented Dec 14, 2015

The Bounded type class is appealing but it currently doesn't give us an Arbitrary[BigInt Refined Positive], right? One way to "fix" this would be to add a low priority implicit Bound[T] from any nt: Numeric[T] with def maxValue = nt.fromInt(Int.MaxValue) and def minValue = nt.fromInt(Int.MinValue). What do you think?

@jedesah
Copy link
Contributor Author

jedesah commented Dec 14, 2015

I think it might be better to provide a (low priority?) instance for BigInt which is Int.MaxValue or Long.MaxValue. I think it's dangerous to assume that an unknown numeric type will support Int.MaxValue. I would rather the user code get a compile error asking for an instance of the Bounded typeclass than a runtime error because nt.fromInt(Int.MaxValue) throws an exception.

Btw, what are the different types for which I need to create instances? Probably all the types in the standard library that have instances for Numeric?

@fthomas
Copy link
Owner

fthomas commented Dec 14, 2015

If I'm handed a nt: Numeric[T] I would never expect that nt.fromInt(x) throws an exception for any x. You would also not expect that Numeric[T].compare throws exceptions, right? But I'm also fine with just an instance for BigInt.

Yes, we should have instances for all types that have Numeric instances in the std lib. A list of these is http://www.scala-lang.org/api/current/index.html#scala.math.Numeric under "Known Subclasses".

@jedesah
Copy link
Contributor Author

jedesah commented Dec 14, 2015

Ah, I assumed it would throw an exception, but I just tested it out with Short and it overflows, so Numeric[Short].fromInt(Int.MaxValue) == -1. But that does not seem a whole lot better. Arguably it's even worse than throwing an exception as it could fail in silent or hard to diagnose ways.

@fthomas
Copy link
Owner

fthomas commented Dec 14, 2015

I also just learned that fromInt is a cast in disguise. So adding a Bounded[BigInt] instance seems to be the better alternative.

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.

None yet

2 participants