Closed drbenvincent closed 1 year ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
View / edit / reply to this conversation on ReviewNB
lucianopaz commented on 2022-10-15T20:07:52Z ----------------------------------------------------------------
I find the names intercept and linear trend highly misleading. The intercept and linear trend should be the names of the random variables that the model learns (which you can beta0 and beta1). These columns are only for a design matrix. I’m not sure what would be a good name for them though. Maybe some generic x_intercept
and x_trend
?
View / edit / reply to this conversation on ReviewNB
lucianopaz commented on 2022-10-15T20:07:53Z ----------------------------------------------------------------
This comment is linked to the one above. It’s very misleading to call the mutable data intercept and linear trend, when they don’t actually represent those two random variables in the model. I suggest that you change the names of the beta variables to intercept and linear trend, and find an alternative for the mutable data. My succession is to leave them as x_intercept
,but I’m not super convinced. If you think of a better alternative, that would be great
drbenvincent commented on 2022-10-16T09:56:08Z ----------------------------------------------------------------
Good feedback. I've removed the intercept 1's as data and just have beta0
now. And I've renamed the other variable to time
which makes much more sense.
View / edit / reply to this conversation on ReviewNB
lucianopaz commented on 2022-10-15T20:07:53Z ----------------------------------------------------------------
Do you want to add something about the prior distribution with respect to the data?
drbenvincent commented on 2022-10-16T10:02:37Z ----------------------------------------------------------------
good point. Done
View / edit / reply to this conversation on ReviewNB
lucianopaz commented on 2022-10-15T20:07:54Z ----------------------------------------------------------------
I don’t know much about this, but is the term “causal impact” or “cumulative causal impact” a precise mathematical notion? Or is it just the expected difference between the observed scenario and the counterfactual one?
drbenvincent commented on 2022-10-16T10:10:00Z ----------------------------------------------------------------
Good point. I've added a brief definition. It is just the difference between the counterfactual prediction and the observed data.
Good feedback. I've removed the intercept 1's as data and just have beta0
now. And I've renamed the other variable to time
which makes much more sense.
View entire conversation on ReviewNB
Good point. I've added a brief definition. It is just the difference between the counterfactual prediction and the observed data.
View entire conversation on ReviewNB
Nice work @drbenvincent! I wanted to ask you to rename the mutable data containers. I find the current name very misleading.
After that, it looks good to merge.
Thanks! I think I've dealt with all the feedback now. I also additionally improved the generated data, which now uses cleaner 'best practice' pandas.
New notebook for the causal inference section.
It purposefully follows a very similar template to the causal inference notebooks.
Happy to receive suggestions for improvement of course.