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

Replace Boston housing analysis with Ames housing #598

Merged
merged 4 commits into from
May 25, 2022

Conversation

fverac
Copy link
Collaborator

@fverac fverac commented Apr 4, 2022

No description provided.

@fverac fverac requested review from ewdillon and kbattocchi April 4, 2022 19:17
@fverac fverac force-pushed the fverac/replaceBostonHousing branch from 999208a to 4171a4b Compare April 8, 2022 15:10
@fverac fverac marked this pull request as ready for review April 8, 2022 15:40
Copy link
Collaborator

@kbattocchi kbattocchi left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in getting this reviewed. Overall, I think it's really good, but I think there's room to improve the connection between the predictive and causal analyses.

Comment on lines 2110 to 2104
" feature_inds= [\n",
" 'HasFireplace',\n",
" 'HasPorch',\n",
" 'HasDeck',\n",
" ],\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why these three features? Overall, this new notebook is good, but one thing that's missing compared to the old one is any finding that one of top predictive features may not be causally important.

Maybe the dataset doesn't support such a finding, and that would be fine to point out too (something like "We find that not only are features X, Y, and Z predictive of high prices, but that even when accounting for other features they appear to be causally linked.") But right now there doesn't seem to be a connection between the predictive and causal analysis.

"cell_type": "markdown",
"metadata": {},
"source": [
"We've seen which features are good predictors of home value, but are these features also causally signficiant? Below we use the CausalAnalysis class to assess the causal effects of certain house characteristics on home value."
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good, but it would be nice to have a parallel statement after calling CausalAnalysis which summarizes whether it agrees or disagrees with the predictive analysis.

Copy link
Collaborator

@kbattocchi kbattocchi left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@fverac fverac force-pushed the fverac/replaceBostonHousing branch from a5d1e49 to 03147bf Compare May 25, 2022 15:41
@fverac fverac merged commit b953f51 into main May 25, 2022
@fverac fverac deleted the fverac/replaceBostonHousing branch May 25, 2022 16:33
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.

2 participants