-
Notifications
You must be signed in to change notification settings - Fork 52
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
Generic default definition for UniformRange #92
Conversation
All right, this is ready for review now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What really bothers me is generic instance for sum types. It uses completely different logic from product case. Generic instances for some class Foo
are mostly used to build instances from Foo
instances of data type's fields. I think such instance would be very surprising for users.
Also I don't think it's very useful I can't imagine any use case where I'd want such instance (it may be lack of imagination, though). It would be better to define no default instances for sums and provide functions to define UniformRange
in terms of FiniteRange
It is most useful and meaningful to derive The proposed deriving strategy mirrors the existing one for It would be interesting to look at a finitely-inhabited data type, for which one would like to have a different instance than produced by |
Laws are very lax and for any single allow many lawful instances. Lawful alone is not enough instance should be meaningful as well. For range it's simple: derived instance is product of ranges. But what is instance for Real problem is I think that we have single strategy: generics (deriving via couldn't be used because of polymorphic Another possibility to provide default implementations using instance UniformRange DayOfWeek where
uniformRM = defaultUniformRMviaEnum
isInRange = defaultIsInRangeviaEnum |
I do not want to argue right now. I removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I do think that proposed default instance for sum types is ill advised. Product types on the contrary are quite reasonable.
gisInRange (x1, x2) x3 && gisInRange (y1, y2) y3 | ||
|
||
isInRangeOrd :: Ord a => (a, a) -> a -> Bool | ||
isInRangeOrd (a, b) x = min a b <= x && x <= max a b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still worry about this even though it's not a change since we had isInRange
with the same definition so the boat has sailed. BTW what do we get with isInRange (127 :: Int8, -128 :: Int8)
- it seems
> fst $ (uniformR (127 :: Int8, -128)) (mkStdGen 42)
fst $ (uniformR (127 :: Int8, -128)) (mkStdGen 42)
-80
I would have expected 127 or -128 as I think of Int8
as a clock - perhaps I shouldn't. Anyhow nothing to do with this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@idontgetoutmuch I don't think you need to worry about it. Your clock or modulo arithmetic thinking is not really valid here. Especially since it would not work universally for all types. For example what would you suggest the behavior to be on: fst $ (uniformR (127 :: Double, -128)) (mkStdGen 42)
?
I think our approach is a perfectly acceptable. We have the law documented:
uniformR (a, b) = uniformR (b, a)
Without that law we cannot make uniformR
a total function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
# 1.3.0 * Improve floating point value generation and avoid degenerate cases: [#172](haskell/random#172) * Add `Uniform` instance for `Maybe` and `Either`: [#167](haskell/random#167) * Add `Seed`, `SeedGen`, `seedSize`, `seedSizeProxy`, `mkSeed` and `unSeed`: [#162](haskell/random#162) * Add `mkSeedFromByteString`, `unSeedToByteString`, `withSeed`, `withSeedM`, `withSeedFile`, `seedGenTypeName`, `nonEmptyToSeed`, `nonEmptyFromSeed`, `withSeedM`, `withSeedMutableGen` and `withSeedMutableGen_` * Add `SplitGen` and `splitGen`: [#160](haskell/random#160) * Add `unifromShuffleList` and `unifromShuffleListM`: [#140](haskell/random#140) * Add `uniformWordR`: [#140](haskell/random#140) * Add `mkStdGen64`: [#155](haskell/random#155) * Add `uniformListRM`, `uniformList`, `uniformListR`, `uniforms` and `uniformRs`: [#154](haskell/random#154) * Add compatibility with recently added `ByteArray` to `base`: [#153](haskell/random#153) * Switch to using `ByteArray` for type class implementation instead of `ShortByteString` * Add `unsafeUniformFillMutableByteArray` to `RandomGen` and a helper function `defaultUnsafeUniformFillMutableByteArray` that makes implementation for most instances easier. * Add `uniformByteArray`, `uniformByteString` and `uniformFillMutableByteArray` * Deprecate `genByteString` in favor of `uniformByteString` * Add `uniformByteArrayM` to `StatefulGen` * Add `uniformByteStringM` and `uniformShortByteStringM` * Deprecate `System.Random.Stateful.uniformShortByteString` in favor of `uniformShortByteStringM` for consistent naming and a future plan of removing it from `StatefulGen` type class * Add a pure `System.Random.uniformShortByteString` generating function. * Deprecate `genShortByteString` in favor of `System.Random.uniformShortByteString` * Expose a helper function `fillByteArrayST`, that can be used for defining implementation for `uniformByteArrayM` * Deprecate `genShortByteStringST` and `genShortByteStringIO` in favor of `fillByteArrayST` * Improve `FrozenGen` interface: [#149](haskell/random#149) * Move `thawGen` from `FreezeGen` into the new `ThawGen` type class. Fixes an issue with an unlawful instance of `StateGen` for `FreezeGen`. * Add `modifyGen` and `overwriteGen` to the `FrozenGen` type class * Switch `splitGenM` to use `SplitGen` and `FrozenGen` instead of deprecated `RandomGenM` * Add `splitMutableGenM` * Switch `randomM` and `randomRM` to use `FrozenGen` instead of `RandomGenM` * Deprecate `RandomGenM` in favor of a more powerful `FrozenGen` * Add `isInRangeOrd` and `isInRangeEnum` that can be used for implementing `isInRange`: [#148](haskell/random#148) * Add `isInRange` to `UniformRange`: [#78](haskell/random#78) * Add default implementation for `uniformRM` using `Generics`: [#92](haskell/random#92)
This is a first cut, need to avoid recursive calls in
instance GFiniteRange (a :+: b)
and add tests.Here is an example: