-
Notifications
You must be signed in to change notification settings - Fork 64
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 #171
Conversation
✅ Deploy Preview for monad-bayes ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Are these large changes e.g. https://github.com/tweag/monad-bayes/pull/171/files#diff-0c0b7688a56339683b2983c70870dfcdc7e9746176d51e7fc6a906fed38528ce because we are still storing the generated html in the repo or is this just the source text for the html and this is a one off?
@@ -2,9 +2,11 @@ | |||
{-# LANGUAGE ImportQualifiedPost #-} |
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.
Why is everything commented out?
@@ -1,35 +1,43 @@ | |||
module Main where |
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 thought we had already made the changes to use a record to contain the various options?
@@ -41,7 +39,7 @@ instance Show Model where | |||
show (HMM xs) = "HMM" ++ show (length xs) | |||
show (LDA xs) = "LDA" ++ show (length $ head xs) | |||
|
|||
buildModel :: MonadInfer m => Model -> m String | |||
buildModel :: MonadInfer n m => Model -> m String |
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.
This extra parameter seems pretty major to me. Can we explain it in the PR itself? I think we should be a bit more disciplined in our PRs going forward (I include myself here and I may be the biggest culprit).
@@ -83,8 +88,8 @@ systems = | |||
[ MonadBayes | |||
] | |||
|
|||
lengthBenchmarks :: Env -> [(Double, Bool)] -> [Double] -> [[T.Text]] -> [Benchmark] | |||
lengthBenchmarks e lrData hmmData ldaData = benchmarks | |||
lengthBenchmarks :: [(Double, Bool)] -> [Double] -> [[T.Text]] -> [Benchmark] |
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.
Not that I loved Env
but why is it being removed?
(n, s) = fold (liftA2 (,) F.length (F.premap (\case True -> 1; False -> 0) F.sum)) points | ||
a' = a + s | ||
b' = b + fromIntegral n - s | ||
-- betaBernoulliAnalytic :: (MonadInfer n m, Foldable t) => BetaParams -> t Bool -> m n Double |
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.
Why is everything commented out? Is this PR not really ready for review?
PR to generalize MonadSample and MonadInfer from Double. Also has beginnings of implementation of HMC, in Control.Monad.Bayes.Traced.Grad. Quite a few things to fix.