Remove `Model` and rewrite `turing_model` #45

devmotion · 2022-02-28T23:36:47Z

This PR implements some of the discussion in #44 and simplifies turing_model.

It also fixes a bug in the current implementation: Currently, turing_model(formula, data; model=M, ...) always returns a model with a Gaussian likelihood, regardless of M.

devmotion · 2022-02-28T23:50:57Z

src/turing_model.jl

+        predictors=size(X, 2),
+        idxs=idxs,
+        n_gr=n_gr,
+        intercept_ranef=intercept_ranef,
+        μ_X=μ_X,
+        σ_X=σ_X,
+        prior=prior,
+        residual=1 / std(y),


Some of these arguments depend on y and X dynamically whereas others are fixed - is this intended? I assume this will be problematic if you reinstantiate the model with new data since then some of the arguments are not synced anymore.

Why do you include all of these arguments in the first place? Do you ever plan to update y or X or change any of the keyword arguments?

That's a great point David. I didn't preview updates on the data. My main focus was to have the user API to be the simplest as possible. We can change it if necessary. My main idea was to mirror rstanarm/brms default priors.

My main focus was to have the user API to be the simplest as possible. We can change it if necessary. My main idea was to mirror rstanarm/brms default priors.

Isn't it simpler to just use model(y, X)? It's still not clear to me why/how these goals imply that you want to use keyword arguments here.

Yes you are right, we can probably move these keywords to the body of the function. We might just keep prior then.

We might just keep prior then.

Because users should be able to call the model with a different prior?

And standardize also, forgot that.

I see.

Another observation: It seems mu_X and sigma_X are not used at all in the models, and there are issues with undefined variables due to the isempty(intercept_ranef) checks.

And standardize also, forgot that.

standardize is not part of the model keyword arguments. Maybe you are referring (also for the priors?) to turing_model? I focused only on the Turing model that this function creates, and the keyword arguments that are currently defined for it.

standardize is not part of the model keyword arguments. Maybe you are referring (also for the priors?) to turing_model?

yes, you are correct. I've made a mess.

storopoli

This is a great PR! Thanks!

One comment: Why not change all Gaussian to Normal?

EDIT: I think I got all Gaussians in the PR changes, but might be worth to have a double check.

docs/src/index.md

docs/src/tutorials/custom_priors.jl

docs/src/tutorials/hierarchical_models.jl

docs/src/tutorials/linear_regression.jl

src/TuringGLM.jl

src/turing_model.jl

test/turing_model.jl

devmotion · 2022-03-01T10:39:39Z

One comment: Why not change all Gaussian to Normal?

Because I assumed you prefer the name Gaussian 😄 Both are equivalent since Distributions defines const Gaussian = Normal.

storopoli · 2022-03-01T10:59:56Z

I prefer, but for our target user might be daunting. I think they would prefer Normal.

I only used Gaussian because the original API did not used Distributions.jl types. So I needed to circumvent that. And the second candidate BellDist was an awful name 😂

test/turing_model.jl

Co-authored-by: Jose Storopoli <[email protected]>

rikhuijzer

Awesome stuff, David 👍 👍

storopoli

If you want to merge, go ahead. We can address the isempty(intercept_ranef) checks in another PR.

yebai · 2022-03-01T15:05:41Z

Thanks for the nice improvements, @devmotion. It’ll be great if we can track performance regression for TuringGLM (e.g. using a notebook similar to tutorials), but that can be done in a separate PR.

rikhuijzer · 2022-03-01T17:56:44Z

It’ll be great if we can track performance regression for TuringGLM (e.g. using a notebook similar to tutorials), but that can be done in a separate PR.

You mean by reporting allocations like in something like fonsp/Pluto.jl#1959? Note that that PR is about compilation time, but runtime could be following a similar procedure. Reporting CPU time doesn't make sense because GitHub Runners run on different CPUs.

storopoli · 2022-03-01T17:58:54Z

Also, we are not doing anything special on TuringGLM.jl we just call Turing.jl under the hood. The overhead might be negligible.

devmotion · 2022-03-01T18:05:59Z

If you want to merge, go ahead. We can address the isempty(intercept_ranef) checks in another PR.

I have many more changes in a local branch but I guess it is cleaner to open a separate PR and do not modify the implementation of the Turing models in this PR.

yebai · 2022-03-01T19:11:02Z

Also, we are not doing anything special on TuringGLM.jl. We just call Turing.jl under the hood. The overhead might be negligible.

That's right. However, the performance of TuringGLM probably depends on the actual turing_model implementation. When we change the turing_model function, it might incur performance changes. So it is good to have a way to track these performance variations. However, I'm not quite sure the best way of automating this.

Reporting CPU time doesn't make sense because GitHub Runners run on different CPUs.

That's correct - not quite sure how to automate performance tracking.

storopoli · 2022-03-01T19:20:22Z

That's right. However, the performance of TuringGLM probably depends on the actual turing_model implementation. When we change the turing_model function, it might incur performance changes. So it is good to have a way to track these performance variations. However, I'm not quite sure the best way of automating this.

This makes total sense! Yes, now I get it.

I've asked @rikhuijzer to help us with this since he is pretty good on that.

Remove Model and rewrite turing_model

fd5512f

devmotion force-pushed the dw/distributions branch from d40f9d0 to fd5512f Compare February 28, 2022 23:38

devmotion commented Feb 28, 2022

View reviewed changes

storopoli reviewed Mar 1, 2022

View reviewed changes

devmotion commented Mar 1, 2022

View reviewed changes

test/turing_model.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

bf6f340

Co-authored-by: Jose Storopoli <[email protected]>

storopoli assigned devmotion and storopoli Mar 1, 2022

rikhuijzer reviewed Mar 1, 2022

View reviewed changes

storopoli approved these changes Mar 1, 2022

View reviewed changes

devmotion merged commit 5731396 into main Mar 1, 2022

devmotion deleted the dw/distributions branch March 1, 2022 18:06

storopoli mentioned this pull request Mar 1, 2022

Track performance of turing_model #47

Closed

Remove Model and rewrite turing_model #45

Remove Model and rewrite turing_model #45

Uh oh!

Conversation

devmotion commented Feb 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

storopoli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devmotion commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

storopoli commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rikhuijzer left a comment

Choose a reason for hiding this comment

Uh oh!

storopoli left a comment

Choose a reason for hiding this comment

Uh oh!

yebai commented Mar 1, 2022

Uh oh!

rikhuijzer commented Mar 1, 2022

Uh oh!

storopoli commented Mar 1, 2022

Uh oh!

devmotion commented Mar 1, 2022

Uh oh!

yebai commented Mar 1, 2022

Uh oh!

storopoli commented Mar 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Remove `Model` and rewrite `turing_model` #45

Remove `Model` and rewrite `turing_model` #45

storopoli left a comment •

edited

Loading

devmotion commented Mar 1, 2022 •

edited

Loading

storopoli commented Mar 1, 2022 •

edited

Loading