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

Iss416 #452

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

Iss416 #452

wants to merge 5 commits into from

Conversation

ridhi96
Copy link

@ridhi96 ridhi96 commented Jan 30, 2025

Mobility metric pull request template

Please include the following points in your PR:

  1. A link to the issue that this PR relates to. You can bring up a list of suggested issues and pull requests within the repository by typing #. Review new way to display data and adjust display of social capital2 metric #416

  2. A description of the content in this pull request.

  • What was changed?
    • Created quarto files for county and place levels containing the updates to social capital 2 metric (economic connectedness)
  • What should the reviewer be focusing on?
    • Change in methodology for calculating metric for both county and place levels
    • Overall structuring of the metric from ZCTA to place level.
  • Is there a logical order to review the files in?
    • You can get a better sense of the updates by reviewing the county file first.
  1. Detail on any issues or flags that the metric reviewer/data-team should be aware of.
    • Metric was updated based on this guidance.

@ridhi96 ridhi96 changed the base branch from main to version2025 January 30, 2025 19:05
Copy link
Collaborator

@jwalsh28 jwalsh28 left a comment

Choose a reason for hiding this comment

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

Thanks for transitinoing this to QMD Rhidi, it is a lot easier to read through and evaluate now. I have mostly harmless comments throughout but a flag for the quality variable for teh city data - I believe there was a slight error here in the prior code.

# and also include total ZCTAs in Place & how many of those partially fall outside the Place


wtd_merged_ec_city <- merged_ec_city |>
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a lot happening in this chunk that I think deserves individual attention. Creation of the zip_total and zips_in varaibles is fairly complex, I would do this in a seperate step and add a bit more explenation.

Copy link
Author

Choose a reason for hiding this comment

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

I have added explanations for each of these code transformations. I think that leaving this chunk of code unchanged is fine because of the grouping level, otherwise will have to do more manipulations to get the data to the place level. I think zip_total could have been achieved without num_ZCTAs_in_place but it's alright, hopefully my explanation makes more sense.

I also changed the way num_ZCTAs_in_place was computed to give more intuition as to what is happening here. Let me know if it makes sense now!

Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks good to me, I think the added detail is helpful

```{r}
#| label: data-quality-flag

# Data Quality 1 = 50% or more of the ZIPs fall mostly (>50%) in the census place
Copy link
Collaborator

Choose a reason for hiding this comment

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

Double-check this step for me. I don't believe the code is actually doing what this explenation says. The variable zips_in is the sum of portion_in which I believe is 1 if the entire ZCTA falls in the place and zero if not. To match the explenation given here for quality I believe you would have to swap the variable mostly_in for portion_in above in the code-chunk labeled collapse-estimates. Please give this a close look.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for pointing this one out! If you refer Tina's code, she left a comment that she never used mostly_in variable after creating it (I have left this comment in the qmd file too). Based on referring the original code, I think Tina intended to use portion_in here. I believe the confusion stems from the word mostly in the quality variable explanation which I have changed to fully now.

The gist is that quality variable considers what percentage of ZCTAs are fully within the boundaries of a Place. So higher the ZCTAs that are encompassed by a Place, the more confidence we have in the transformed economic connectedness value linked with that place. Does this make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this makes a lot more sense now, great clarification!

@ridhi96 ridhi96 requested a review from jwalsh28 February 25, 2025 16:01
@ridhi96
Copy link
Author

ridhi96 commented Feb 25, 2025

Hi @jwalsh28! I updated the files based on your review and added responses to your comments. I have tried to provide answers to some of the questions you had about methodology. Please let me know if you have any more thoughts/questions. Thank you!

Copy link
Collaborator

@jwalsh28 jwalsh28 left a comment

Choose a reason for hiding this comment

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

This looks good Ridhi! I left one comment open on the qnorm decision to see if Aaron has a chance to chime in. Otherwise I think this is complete.

),
# calculate lower bound from standard error
share_economic_connectedness_lb =
share_economic_connectedness - ( qnorm(0.975) * ec_se_county_updated ),
Copy link
Collaborator

Choose a reason for hiding this comment

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

@awunderground Flagging for you if you have a second to share any thoughts here, though I think Ridhi's explanation makes sense.

```{r}
#| label: data-quality-flag

# Data Quality 1 = 50% or more of the ZIPs fall mostly (>50%) in the census place
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this makes a lot more sense now, great clarification!

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