Conversation
Shimuuar
left a comment
There was a problem hiding this comment.
One problem with exclusive intervals is they inevitably bring botttoms with themselves. It's trivival to construct intervals which has no points in them: a < x < a, for integral types there's even more choice: a < x < a+1.
| => (Bound 'Exclusive a, Bound 'Exclusive a) | ||
| -> g s | ||
| -> m a | ||
| uniformIntegralExcExcRM (Exc l, Exc u) = uniformRM (Inc (l + 1), Inc (u - 1)) |
There was a problem hiding this comment.
This function assumes that l < u while old uniformRM implementation ensured that uniformRM (a,b) = uniformRM (b,a). Not to mention possible over/underflow problems
There was a problem hiding this comment.
I am glad you caught it, but I left it out on purpose, because it is a separate issue, see my comment: #113 (comment)
| -- @since 1.2 | ||
| class UniformRange a where | ||
| uniformRM :: MonadRandom g s m => (a, a) -> g s -> m a | ||
| class UniformRange (l :: Clusivity) (u :: Clusivity) a where |
There was a problem hiding this comment.
Is Clusivity actually needed? Instead of having isomorphism between types constructors and promoted constructors why not to use them directly:
class UniformRange l u a where
uniformRM :: MonadRandom g s m => (l a, u a) -> g s -> m aIt's possible to make yet another step and use single type for interval:
class UniformRange interval a where
uniformRM :: MonadRandom g s m => interval a -> g s -> m aThis could be generalized to all sorts of sets: open sets for bounded types, disks and rings for complex numbers etc. We can even recover plain uniformRM (a,b) notation by defining say:
instance (UniformRange Inclusive a, a' ~ a) => UniformRange ((,) a') a where
...There was a problem hiding this comment.
Clusivity kind is needed to prevent precisely this abuse that you are suggesting.
There was a problem hiding this comment.
I think this is not abuse but pretty natural generalization of uniform sampling from interval of values of type a to uniform sampling from set of values of type a.
There was a problem hiding this comment.
That is a very subjective: "pretty natural generalization". In any case this is way too general. I don't think giving too much freedom is a good thing here. It is possible to create nonsense things like this for example: instance UniformRange Maybe Float
There was a problem hiding this comment.
It's possible to define meaningless instances for basically any type class. Usually it's not possible to define rigorously what is meaningful either so it has to be deferred to programmer who define instance.
I think that if you add type variable to class you should to exploit it to the fullest. Artificially limiting its power seems wrong to me.
There was a problem hiding this comment.
Nothing could be easier! Encoding is simple:
data IntervalIncInc a = IntervalIncInc !a !a
data IntervalExcExc a = IntervalExcExc !a !a
instance UnformRandom IntervalIncInc Int where
Only problem is inventing decent names.
On more serious note. Is it even possible to to have type where you can define generation in one interval type but not in another? Generally one needs to have only finitely many point in the interval to sample from it. In this case exclusive intervals could be trivially expressed via inclusive
If conjecture above is true then UniformRange with 4 methods will work.
There was a problem hiding this comment.
You know, I think you are right. Giving it a bit more thought and your example convinces me that this approach seems better. In that case, do you think UniformRange is still a good name for the class then, because it becomes more general than just ranges (intervals) and, as you said, it can be used with subsets, rings, disks etc. What do you think a proper name for it could be? UniformRegion maybe?
class UniformRegion r a where
uniformRM :: MonadRandom g s m => r a -> g s -> m aThere was a problem hiding this comment.
No UniformRange name implies that we generate values in range from a to b. And type class became more general that that. UniformRegion looks like fine name
BTW it's possible to keep name UniformRange:
type UniformRange = UniformRegion IntervalJust checked with mtl that this approach works
There was a problem hiding this comment.
I am always very much against type synonyms for public APIs because all they do is add complexity without any benefit. The only benefit type synonyms can have is reducing number characters to type and documentation, but I am very much convinced that neither of those benefits is worth of adding an extra name for users to learn about.
Let's keep UniformRange name and not worry about it. I'll update this PR over the weekend to reflect your suggested approach
There was a problem hiding this comment.
I am always very much against type synonyms for public APIs
I know. But bringing up yet another idea? That I can't resist. For the record: I think UniformRegion is better name but UniformRange is acceptable
|
One alternative approach could be to have a proper representation of the concept of "range" itself. This is a thing in Guava, for example. #115 (comment) got me thinking about this:
The only cause of failure that comes to mind right now is that |
|
@curiousleo This all depends on the clusivity we will decide to support:
If we are not going to support any range other than inclusive on both sides: Another failure that is up for a debate is the floating point edge cases: should Last and not least, because UniformRange is a class we do not know ahead of time what are the failures a particular type can have. This is an interesting idea. We can explore it further.
Regardless of how we gonna approach the clusivity implementation and the API for it, I want to wait on your work with floating point number generation and what will be the final solution there. From #113 it is still not clear to me what clusivity will be available for |
|
An interesting consequence of your suggestion of introducing a "Range" concept, would remove a need for |
|
Requiring special nonempty ranges just moves problem of validation to construction of ranges. There's also question of ergonomics. How to do validation when range is constructed at uniformRM call site:
There're types for which aren't comparable, for example rotations in plane. It's however doable with special range for all possible values instance UnformRegion Everything X where
uniormRM = ...
uniformM :: (MonadRandom g s m, UniformRegion Everyting a) => g s -> m a
uniformM = uniformRM Everything |
|
As you noted "... moves problem of validation to construction of ranges.", which means range construction would become monadic with
So this would become something like this: "Everything" was exactly what I had in mind:
|
|
In light of #113 (comment) and #113 (comment), I think we can shelve this for now as we have decided not to focus on for v1.2. The discussion that's already happened here is still valuable once we pick up clusivity API work again, so I'm happy to leave this open - as you prefer. If so, perhaps mark it as |
|
Somewhat related discussion, for future reference: https://marcosh.github.io/post/2020/05/04/intervals-and-their-relations.html and https://github.com/marcosh/availer deal with modeling https://en.wikipedia.org/wiki/Allen%27s_interval_algebra in Haskell. |
This is the clusivity implementation I presented on slack. This PR is based on #103 so it needs to be merged first
While implementing it I came to like this approach even more. Now a user will have to think about the actual range instead of relying on documentation about the ranges to tell the user about the clusivity to expect on ranges.
So, now in order to accommodate fully inclusive range for floating point numbers in #102 all that is required is the addition of
instance UniformRange 'Inclusive 'Inclusive Double(same for Float)If we never figure out fully exclusive range for floating point numbers, that is ok, it will be conveyed to the user by the type system that we can't do it:
"no instance for 'instance UniformRange 'Exclusive 'Exclusive Double'"FYI, I checked this PR for any performance degradation and there was no affect at all. So all we get is just the type safety, which is great.