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

Remove net_wealth from PSID, update data folder, and code formatting in psid_data_setup.py #23

Closed
wants to merge 4 commits into from

Conversation

MaxGhenis
Copy link
Contributor

@MaxGhenis MaxGhenis commented Feb 6, 2021

I messed up #15 after getting behind master and accidentally adding a big file, so trying again. This PR:

The only substantive changes are in line 25 and 148; everything else is code formatting (sorry these aren't in separate commits).

@MaxGhenis
Copy link
Contributor Author

MaxGhenis commented Feb 6, 2021

I also verified that the script runs. It produced the following files in ogusa_calibrate:

first_stage_reg_results.pkl
hh_id_two_statuses.csv
psid_lifetime_income.pkl
psid_to_check.csv

I added ogusa_calibration/*.pkl and ogusa_calibration/*.csv to the .gitignore, though we probably want to move these files elsewhere, and/or potentially include them. Only psid_lifetime_income.pkl exceeds GitHub's 100MB limit (130MB), and some other file compression types might get that below the limit.

@MaxGhenis
Copy link
Contributor Author

@jdebacker please let me know if you'd like more info on this to decide on merging.

This script takes PSID data created from psid_download.R and:
1) Creates variables at the "tax filing unit" (equal to family unit in
PSID since there is no info on the filing status chosen).
2) Selects a sample of observations to work with (e.g., dropping very
old, very low income, etc.).
3) Computes a measure of lifetime income and places each household into
a lifetime income percentile group
'''
"""
Copy link
Member

Choose a reason for hiding this comment

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

Are the double quotes everywhere a black thing @MaxGhenis? You're killing me with these :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep will definitely make these separate commits in the future!

@jdebacker
Copy link
Member

@MaxGhenis This is looking good to me. I especially like including those files in the git ignore. I don't think they need to be checked into the repo.

@rickecon
Copy link
Member

rickecon commented Jun 6, 2021

@MaxGhenis @jdebacker . I think this PR is ready to merge. But I don't understand the "conflicts that must be resolved" above. When I click on Resolve conflicts, it shows me a version of the psid_data_setup.py file that is not consistent with the changes shown in the "Files changed" tab. Maybe this is just a case where Max's branch is behind the current state of the repo, and he needs to do a git fetch/merge/resolve conflicts on his end. Any clarification here would be helpful.

@jdebacker
Copy link
Member

@MaxGhenis When conflicts are resolved on this branch, I'm very happy to merged this. B/c wealth variables are important and the psidR package no longer can be used for them, I may check back into this repo the old PSID data with wealth, just in case we need it. But I like the updates here so that things work with the latest psidR package.

@jdebacker jdebacker closed this Oct 23, 2023
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.

3 participants