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

Generalize from double: with type family #177

Draft
wants to merge 172 commits into
base: master
Choose a base branch
from

Conversation

reubenharry
Copy link
Contributor

No description provided.

@netlify
Copy link

netlify bot commented Aug 29, 2022

Deploy Preview for monad-bayes ready!

Name Link
🔨 Latest commit 6cb62a0
🔍 Latest deploy log https://app.netlify.com/sites/monad-bayes/deploys/630cbc5f5c634e0009501197
😎 Deploy Preview https://deploy-preview-177--monad-bayes.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@reubenharry reubenharry mentioned this pull request Aug 29, 2022
9 tasks

-- | Draw from a uniform distribution.
uniform ::
-- | lower bound a
Double ->
(Real m) ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
(Real m) ->
Real m ->

@reubenharry
Copy link
Contributor Author

@mknorps @idontgetoutmuch If we still want HMC, we'll need to merge/rebase this branch: it generalizes from Double in a way that allows for automatic differentiation.

@turion
Copy link
Collaborator

turion commented Oct 19, 2022

@reubenharry What is missing? Do you need any help? We're working around statistics only supporting Double right now, right?

@idontgetoutmuch
Copy link
Member

Do we need statistics? Would mwc-random be enough? I am sure @Shimuuar would accept a PR to generalise the types e.g. here https://hackage.haskell.org/package/mwc-random-0.15.0.2/docs/System-Random-MWC-Distributions.html provided we didn't affect performance.

BTW we should also check performance before and after generalising Double to Num a (or whatever).

@turion
Copy link
Collaborator

turion commented Oct 19, 2022

mwc-random also pins the implementations of the distributions to Double.

@idontgetoutmuch
Copy link
Member

mwc-random also pins the implementations of the distributions to Double.

I understand but I think the maintainer would be sympathetic to generalising it provided we did not impact performance.

@reubenharry
Copy link
Contributor Author

@turion So the ad library supports the inverse error function, so we can at least currently do normal distributions without statistics. Less so for the other distributions in the MonadSample class. I don't know if we mind the sampler working in terms of Double, but if we do, we'd indeed want to request a change to mwc-random or move to something else.

@Shimuuar
Copy link

I'm not against generalization but it's kind of tricky. Some generators would be easy: exponential do not require anything beyond log. Others may be trickier. Generator for normal distribution uses ziggurat method and corresponding tables are only available for Doubles, some may use math-functions which provide only versions for Doubles as well

@turion
Copy link
Collaborator

turion commented Oct 20, 2022

Generator for normal distribution uses ziggurat method and corresponding tables are only available for Doubles

Why is that? It seems to me that the table is calculated in Haskell as well, so one could calculate several tables, possibly creating a type class for this.

@Shimuuar
Copy link

Why ziggurat? It's fast. Last time I run benchmarks (several years before random 1.2) it was only tens of percent slower than generation of Double. I didn't compare it with inversion method though.

Creating type class is possible but that's road leads to type class per distribution.

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.

4 participants