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

Add more intro material quick start #436

Merged
merged 9 commits into from
May 23, 2024

Conversation

juanitorduz
Copy link
Contributor

Related to #435

@juanitorduz
Copy link
Contributor Author

@s3alfisc I recommend you enable the https://www.reviewnb.com/ app (it is free for public repos!) to review notebooks :)

@juanitorduz juanitorduz marked this pull request as draft May 13, 2024 10:47
@juanitorduz
Copy link
Contributor Author

@s3alfisc I started working on #435. What do you think about the intro section? I want to do something similar for the rest of the sections. Hence, I want some initial feedback :)

For such purpose, I need to re-run all the notebook (also important for all users). However, I can not access C:/Users/alexa/Downloads/census2000_5pc.dta ;) Is there a way to fetch this from other source or maybe even add it into the repository (If the corresponding license allows it). I think is important for users to be able to run the quickstart end-to-end.

Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.00%. Comparing base (35b47e9) to head (6ecc8de).
Report is 81 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
- Coverage   85.79%   83.00%   -2.79%     
==========================================
  Files          38       51      +13     
  Lines        3421     4243     +822     
==========================================
+ Hits         2935     3522     +587     
- Misses        486      721     +235     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@s3alfisc
Copy link
Member

I've activated the reviewnb tool. Very nice! Trivago is even mentioned in the list of customers, I should ask procurement if we actually have a professional license =)

Beyond that I think the intro is excellent. Clearly describes the problem and I also like the sketch of the solution by demeaning! Thanks so much! =)

The reason why you're results don't match exactly is that you forgot to demean the dependent variable:

fit_demeaned = pf.feols(
    fml="Y_demeaned ~ X1_demeaned",
    data=data.assign(
        Y_demeaned=lambda df: df["Y"] - df.groupby("f1")["Y"].transform("mean")
        X1_demeaned=lambda df: df["X1"] - df.groupby("f1")["X1"].transform("mean")
    ),
    vcov="HC1",
)

fit_demeaned.summary()

The non-existing file can be downloaded from http://www.damianclarke.net/stata/census2000_5pc.dta but

import pandas as pd 
df = pd.read_stata("www.damianclarke.net/stata/census2000_5pc.dta")

fails for some reasons. But the file is super big, so maybe we should add a warning about that?

@juanitorduz
Copy link
Contributor Author

@s3alfisc pd.read_stata("http://www.damianclarke.net/stata/census2000_5pc.dta") seems to work :)

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@juanitorduz juanitorduz marked this pull request as ready for review May 17, 2024 09:59
@juanitorduz
Copy link
Contributor Author

@s3alfisc I think this one is ready for a first review round :)

@s3alfisc
Copy link
Member

Thanks - I think it looks really good but I still left a lot of comments 😄

@juanitorduz
Copy link
Contributor Author

Thanks - I think it looks really good but I still left a lot of comments 😄

Hey @s3alfisc I can not see your comments 😅 . Can you please send the again? 🙏

@s3alfisc
Copy link
Member

s3alfisc commented May 19, 2024

Oh - I left the comments in the review notebook. Can you see them there?

Basically, I wonder about how to simply and intuitively define what a fixed effect is but think you have done a pretty good job, actually; and then I also wonder if it would help to rename the columns in pd.get_data() or to create a new synthetic data set so that the column names are aligned with the story in quickstart? E.g. we could think about creating a standard DiD data set with unit and time fixed effects, or a dataset for a standard AKM-regression with worker and firm fixed effects. Or maybe you have a better example from the tech world (e.g. maybe a cluster randomized trial?)? Column names of "f1" and "f2" are probably good enough for my internal testing but likely not too pedagogical 😄

@juanitorduz
Copy link
Contributor Author

@s3alfisc ok! Got it! It would be nice to see notebook comments. Could you try again? Sometimes one need a to hit the “finish review” button 😄

Copy link
Member

@s3alfisc s3alfisc left a comment

Choose a reason for hiding this comment

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

placeholder to refresh review nb

@s3alfisc
Copy link
Member

Looks like all my comments are gone =/ Will start a second round then!

@s3alfisc s3alfisc self-requested a review May 19, 2024 16:04
docs/quickstart.ipynb Outdated Show resolved Hide resolved
docs/quickstart.ipynb Outdated Show resolved Hide resolved
docs/quickstart.ipynb Outdated Show resolved Hide resolved
docs/quickstart.ipynb Outdated Show resolved Hide resolved
Copy link
Member

@s3alfisc s3alfisc left a comment

Choose a reason for hiding this comment

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

Many comments within the ipynb now - I did not want to risk losing all of them in the review nb again 😅

@juanitorduz
Copy link
Contributor Author

Ok I started with the feedback in eadcbf8 . I will continue in the next days so that we don't delay this PR for long :D

@juanitorduz
Copy link
Contributor Author

ok @s3alfisc I think we are ready for another review round :)

@juanitorduz juanitorduz requested a review from s3alfisc May 23, 2024 08:17
docs/quickstart.ipynb Outdated Show resolved Hide resolved
docs/quickstart.ipynb Outdated Show resolved Hide resolved
@s3alfisc
Copy link
Member

This looks excellent @juanitorduz! I found two more small typos. Beyond that, I think this PR is ready to be merged! I really appreciate the added section on multiple estimations & the section on "brave and true". I think the intro has improved a lot. Vielen Dank =)

@juanitorduz
Copy link
Contributor Author

This looks excellent @juanitorduz! I found two more small typos. Beyond that, I think this PR is ready to be merged! I really appreciate the added section on multiple estimations & the section on "brave and true". I think the intro has improved a lot. Vielen Dank =)

Fixed!

@s3alfisc
Copy link
Member

Ah wow you have been faster then me =D I will merge it after the CI run. Thank you!

@juanitorduz
Copy link
Contributor Author

@s3alfisc thank you for the comments and feedback!

BTW: You can ad reviews directly in the ReviewNB notebook view as well ;

@@ -6,45 +6,151 @@
"source": [
Copy link
Contributor Author

@juanitorduz juanitorduz May 23, 2024

Choose a reason for hiding this comment

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

Like this! XD


Reply via ReviewNB

@juanitorduz
Copy link
Contributor Author

Click on the ReviewNB and you will see the comment on the cell ;)

@@ -6,45 +6,151 @@
"source": [
Copy link
Member

@s3alfisc s3alfisc May 23, 2024

Choose a reason for hiding this comment

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

Trying out a review from the review nb (I failed with this the other day) - I think this is my favorite new section =)


Reply via ReviewNB

@s3alfisc
Copy link
Member

I now realize what I did wrong the other day - I added a lot of comments but the review was only "pending", I never clicked on "post to github". That's why all my initial comments got lost 🌞

@s3alfisc s3alfisc merged commit ee73027 into py-econometrics:master May 23, 2024
8 checks passed
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