search

haskell-numerics / random-fu

A suite of Haskell libraries for representing, manipulating, and sampling random variables
42 stars 21 forks source link

Exported the Categorical distribution type. #3

Closed absz closed 12 years ago

absz commented 12 years ago

This was the only distribution whose data type wasn't exported, so I made things more regular. I exported the constructor as well, since the rest of the types have exported constructors.

I actually just needed this in a project I'm working on; as it stands, you can't declare a data type which has a field whose type is Categorical p a.

absz commented 12 years ago

I also added a Read instance (like Map's) and removed a spurious Ord a constraint on fromWeightedList :: Fractional p => [(p,a)] -> Categorical p a. (It is necessary for fromObservations, which is presumably where it came from.)

mokus0 commented 12 years ago

Ah, thank you - not exporting the type was an unintentional error, and the Read instance is something I had put off and never returned to. Hiding the type constructor was intentional, though. The representation has changed in the not too distant past, and there are enough different ways to represent the type I felt it best to offer them all on equal footing.

Also the representation actually used is not one that the user is likely to expect (and the documentation glosses over that because it's not exposed, so I'll have to update that too if the constructor is to be exposed). Specifically, the number associated with each element in the vector is the sum of its weight and those of all elements before it, so that sampling can be done with a binary search.

Do you need access to that representation, or think that others would? If so I don't mind exposing it but I think I still would prefer to do so by creating wrapper functions rather than exposing the data constructor itself.

mokus0 commented 12 years ago

For now I've re-hidden the data constructor only, as I had intended to do originally. Let me know if this works for you and if so I'll upload it to Hackage.

absz commented 12 years ago

That works great, thanks. I think you're right that hiding the constructor makes sense; I had just opted to copy what the other distributions were doing, but the existence of toList makes hiding the constructor entirely non-problematic. I've checked, and the new version works perfectly for what I'm working on.

mokus0 commented 12 years ago

Glad to hear it, I'll pack it up and upload it. It bothered me a bit too that it was the only distribution type with its constructor hidden, but I think it just makes more sense that way.