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

create data layer for worldpop_age_sex #64

Merged
merged 14 commits into from
Oct 24, 2024
Merged

Conversation

tedw0ng
Copy link
Member

@tedw0ng tedw0ng commented Aug 23, 2024

Create data layer for Worldpop disaggregated data by age and sex groups.
Question: the regular worldpop layer has a .mean() line. Should it be sum()? Same question would apply to this new layer.

@tedw0ng tedw0ng requested a review from weiqi-tori August 23, 2024 07:52
@S-AI-F
Copy link
Member

S-AI-F commented Aug 23, 2024

@tedw0ng Agree that it should be sum() instead of mean(). I checked the original notebook and we are using also mean(): https://github.com/wri/cities-indicators/blob/emackres-patch-1/notebooks/extract-layers/extract-WorldPop-OpenSpaceTreeAccess.ipynb
I don't remember any reason for that. I think we should change this!

@S-AI-F
Copy link
Member

S-AI-F commented Aug 23, 2024

I also see that worldpop provides the data through a specific api now: https://www.worldpop.org/sdi/introapi/
Should we consider changing the data access?

@tedw0ng
Copy link
Member Author

tedw0ng commented Aug 23, 2024

I think the Worldpop API only returns JSON data, not rasters. That might be okay for indicators, but not for data layers.

@S-AI-F
Copy link
Member

S-AI-F commented Aug 26, 2024

It actually serves also the url to get the tiff files but at the national level. So it is probably better if we keep extracting the data from GEE to get only areas of interest
image

Copy link
Member

@weiqi-tori weiqi-tori left a comment

Choose a reason for hiding this comment

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

Could add a test for the layer to check if it works well



class WorldPop(Layer):
def __init__(self, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

agesex_classes should be added as an input parameter for __init__()

Copy link
Member Author

Choose a reason for hiding this comment

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

added agesex_class as input parameter

city_metrix/layers/worldpop_age_sex_class.py Outdated Show resolved Hide resolved
city_metrix/layers/worldpop_age_sex_class.py Outdated Show resolved Hide resolved
@weiqi-tori
Copy link
Member

The layer and test standard structure changed since this layer was created, so I fixed the conflict and updated the layer.

@tedw0ng: Please pull the branch with the latest update, and test if the layer functions as you expected. One note is that I switched the aggregation for image collection back to mean(). It's not aggregate the pixels within the bbox, it's for the image collection aggregating to an image when there is overlap between images.

@weiqi-tori weiqi-tori requested a review from chrowe September 25, 2024 09:00
@chrowe chrowe requested review from S-AI-F and removed request for S-AI-F and chrowe October 7, 2024 21:06
@S-AI-F
Copy link
Member

S-AI-F commented Oct 15, 2024

@weiqi-tori @chrowe

Checked in this notebook and works globally well: https://colab.research.google.com/drive/1VtDgtQ_BVRh7h0QJ56ryfHJCByLs5Rqu#scrollTo=uXq9pqNOH7fw

Proposed changes:

  • Would be great if we add an error checking on the "year" parameter. It should be 2020 and if the user specify different year, show an error message: "the selected year is not available". Now it works with any year value
  • Can we add in the code the available age_sex list of values: [M_0, M_1, M_5, M_10, M_15, M_20, M_25, M_30, M_35, M_40, M_45, M_50, M_55, M_60, M_65, M_70, M_75, M_80, F_0, F_1, F_5, F_10, F_15, F_20, F_25, F_30, F_35, F_40, F_45, F_50, F_55, F_60, F_65, F_70, F_75, F_80]

super().__init__(**kwargs)
self.agesex_classes = agesex_classes
Copy link
Member

Choose a reason for hiding this comment

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

add comment n possible list of values here just for documentation: [M_0, M_1, M_5, M_10, M_15, M_20, M_25, M_30, M_35, M_40, M_45, M_50, M_55, M_60, M_65, M_70, M_75, M_80, F_0, F_1, F_5, F_10, F_15, F_20, F_25, F_30, F_35, F_40, F_45, F_50, F_55, F_60, F_65, F_70, F_75, F_80]

Copy link
Member

Choose a reason for hiding this comment

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

@weiqi-tori weiqi-tori merged commit fcb6570 into main Oct 24, 2024
1 check passed
@weiqi-tori weiqi-tori deleted the add_worldpop_agesex_classes branch October 24, 2024 06:06
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