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

Feature/wrapper_functions_api #500

Merged
merged 12 commits into from
Jul 6, 2022
Merged

Conversation

zeliest
Copy link
Collaborator

@zeliest zeliest commented Jun 30, 2022

Add a wrapper function to get centroids from the API
change get_litpop_default() to get_litpop()
add more information to error message when no dataset are found in the wrapper functions
update notebook

Copy link
Member

@chahank chahank left a comment

Choose a reason for hiding this comment

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

Great addition! I suggest some minor changes to the available variables.

@@ -844,6 +844,44 @@ def get_litpop_default(self, country=None, dump_dir=SYSTEM_DIR):
raise ValueError("country must be string or list of strings")
return self.get_exposures(exposures_type='litpop', dump_dir=dump_dir, properties=properties)

def get_centroids(self, res_arcsec_land=150, res_arcsec_ocean=1800, extent=None, reg_id=None, country_iso3alpha=None,
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure whether the attribute reg_id is very sensible here. This is a number that cannot really be known by a user of the API right? Anyway, the reg_id is the same as the country_iso3alpha correct? I would thus keep only one of the two, and only the one that is really understandable by a user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay, it was just to follow the centroids.select() arguments. but probably country_iso3alpha is sufficient

resolution for ocean centroids in arcsec. Default is 1800
reg_id : int
region to filter according to region_id values
extent : tuple
Copy link
Member

Choose a reason for hiding this comment

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

Should we put the default to have min_lat = -60 and max_lat = 60? Also, the default is not explained. Is it the whole world?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes for now I put the whole world, I would keep it like that. Because for most hazards it makes sense to have the entire globe

Copy link
Member

Choose a reason for hiding this comment

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

But is it cut at high/low latitudes or not? This would be important to know. The default extent could then be with +-60degrees?

If min_lon > lon_max, the extend crosses the antimeridian and is
[lon_max, 180] + [-180, lon_min]
Borders are inclusive.
dump_dir : str
Copy link
Member

Choose a reason for hiding this comment

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

The country_iso3alpha is missing (also the reg_id, but I would remove that one probably, or make it clear that it is simply the country iso3 numeric.

Returns
-------
climada.hazard.centroids.Centroids
global centroids from the api
Copy link
Member

Choose a reason for hiding this comment

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

It is not always global, only by default.

Comment on lines 875 to 876
dataset = self.get_dataset_info("centroids", properties={'res_arcsec_land':res_arcsec_land,
'res_arcsec_ocean':res_arcsec_ocean,'extent':extent_property})
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a check whether the data exist would be useful? Because the error message from get_dataset_info is not very useful if for instance I say res_arcsec_land=2500 and it does not exist...

Comment on lines 872 to 873
res_arcsec_land = str(res_arcsec_land)
res_arcsec_ocean = str(res_arcsec_ocean)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
res_arcsec_land = str(res_arcsec_land)
res_arcsec_ocean = str(res_arcsec_ocean)

Comment on lines 875 to 876
dataset = self.get_dataset_info("centroids", properties={'res_arcsec_land':res_arcsec_land,
'res_arcsec_ocean':res_arcsec_ocean,'extent':extent_property})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
dataset = self.get_dataset_info("centroids", properties={'res_arcsec_land':res_arcsec_land,
'res_arcsec_ocean':res_arcsec_ocean,'extent':extent_property})
dataset = self.get_dataset_info("centroids", properties={'res_arcsec_land':str(res_arcsec_land),
'res_arcsec_ocean':st(res_arcsec_ocean),'extent':extent_property})

Comment on lines 880 to 881
if country_iso3alpha:
reg_id = pycountry.countries.get(alpha_3=country_iso3alpha).numeric
Copy link
Collaborator

Choose a reason for hiding this comment

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

In get_litpop_default, there is also a country argument, but it's treated with more tolerance, it can be anything that pycountry.countries.lookup makes sense of, and even a list.
Perhaps the two methods could be aligned with regard to treating countries?

@zeliest zeliest changed the title Feature/get centroids api Feature/wrapper_functions_api Jul 1, 2022
zeliest and others added 4 commits July 5, 2022 09:25
- adjust test_get_exposures for v2 dataset
- adjust download faiilure for multi-version datasets
- move NoResult error message writing into get_dataset
@emanuel-schmid emanuel-schmid merged commit 1eb314a into develop Jul 6, 2022
@emanuel-schmid emanuel-schmid deleted the feature/get_centroids_api branch July 6, 2022 15:45
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