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

Initial PR for preschool update, adding historical years #429

Open
wants to merge 14 commits into
base: version2025
Choose a base branch
from

Conversation

jwalsh28
Copy link
Collaborator

@jwalsh28 jwalsh28 commented Nov 8, 2024

No description provided.

jwalsh28 and others added 3 commits November 8, 2024 10:04
…accepted by the IPUMSR functions which are key to organizing the data in a way that makes the rest of the code work. Need to find a better way to make IPUMSR functions interact with AWS
@jwalsh28
Copy link
Collaborator Author

@ridhi96 See my last commit. I'm having trouble with making aws.s3 compatible with IPUMSR. Let's chat about potential solutions.

@cdsolari cdsolari requested a review from ridhi96 February 5, 2025 21:15
@ridhi96 ridhi96 marked this pull request as ready for review February 18, 2025 18:21
Copy link

@ridhi96 ridhi96 left a comment

Choose a reason for hiding this comment

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

Hi @jwalsh28! I have reviewed and added my feedback comments. Please let me know if you have any questions on this, thank you!

Also, I am not quite sure what to do about the HTML files.


*User note* This program depends on extracts from the IPUMS ACS API interface. If you are updating the extract please remember to update the note of the date of latest extract in the Housekeeping section of this program.

*Internal users* If you are an internal tester using the AWS feature of this program remember to enter the passkey into your environment using Sys.setenv(). Authentication steps are nicely explained in this [blog](https://www.gormanalysis.com/blog/connecting-to-aws-s3-with-r/). You will have to request access to the access key.
Copy link

Choose a reason for hiding this comment

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

We should probably mention that the access key can be requested by contacting the AWS Admin. Don't want to point to internal resources at Urban but give internal users an idea where to look for it.



# DO NOT PUSH YOUR API KEY. You only have to run this once and then comment it out as below.
#set_ipums_api_key("Your KEY", save = TRUE)
Copy link

Choose a reason for hiding this comment

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

I think it's a little risky to ask the user to add in their key and then comment the code again. If they forget to remove the key after and just comment the code, it will still get pushed to GitHub. Maybe we can direct the user to this function and tell them to execute it in the console as part of set up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For external users I think there is low risk of this because they won't be pushing this back to GitHub. I see risk potentially for future metric leads but think it is fairly low.


Date of IPUMS extract. Denote the latest date that the IPUMS extract was changed and pulled both for the overall data and the repweights. The recommended date notation format is "mm_dd_yy".

For internal reviewers, if you plan to utilize AWS keep this date as it was last enetered by the metric lead unless you intend to change the extract.
Copy link

Choose a reason for hiding this comment

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

*entered


```{r}

acs_surveys <- list(paste0("us", params$year, "c"))
Copy link

Choose a reason for hiding this comment

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

It will be useful for future updates to mention that params$year needs to be updated in the YAML parameters at the top.

@@ -653,7 +664,7 @@ metrics_education_all <- results_all_expand %>%
### Write out intermediate data

Copy link

Choose a reason for hiding this comment

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

In a lot of places in this code file counties are mentioned (e.g. line 557, 587 etc.) when it should be places. Please check and update.

toc_float: true
embed-resources: true
code-fold: show
format:html
execute:
warning: false
editor_options:
chunk_output_type: console
---

Copy link

Choose a reason for hiding this comment

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

I think it would be useful to explain at the top what the intent of this file and why we are generating place data like this.

@jwalsh28 jwalsh28 requested a review from ridhi96 February 27, 2025 04:21
Copy link

@ridhi96 ridhi96 left a comment

Choose a reason for hiding this comment

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

Hi @jwalsh28, just left some minor comments that shouldn't take long to fix. The code looks good, thank you!

```

Look at the distribution of survey samples in the data. The number of unique samples in the data should match the number of surveys selected in the `extract_ipums()` function above.
Look at the survey year of the IPUMS data. The survey should year should match what was selected in the `extract_ipums()` function above.
Copy link

Choose a reason for hiding this comment

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

Typo here: "The survey should year should"


## Render QMD Years

Select which years of new data you want to run the preschool place calculate file for.
Copy link

Choose a reason for hiding this comment

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

I think this file generates county files too. Please add a short explanation of the purpose of this file so any future metric leads can figure out if they need to update this. Thanks!

@jwalsh28 jwalsh28 requested a review from ridhi96 February 28, 2025 19:29
Copy link

@ridhi96 ridhi96 left a comment

Choose a reason for hiding this comment

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

Hi @jwalsh28! Thank you for updating the reviewed files. When you changed the year to "2023", the graphs showing the prek_attendance metric in preschool_place.qmd and preschool_county.qmd files need to be updated to display the year "2023" as they still show "2021".

@jwalsh28
Copy link
Collaborator Author

jwalsh28 commented Mar 3, 2025

@ridhi96 I updated the graphics in the validity section to use the year in the Params argument, given how the program is set up this should always reflect the latest year.

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