exposures-init-geometry #890
Conversation
…itude have been eliminated
…itude have been eliminated
…itude have been eliminated
considering dataframes with geometry column instead fo lat/lon
# Conflicts: # climada/util/coordinates.py
consequently use region_id property where applicable
# Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Great job !
A few comments in addition to the suggestion:
- I think it would be important to give a more detailed description of what this PR does here and in the changelog. From what I understand, there are more changes than initializing the geometry within the init of Exposure.
- A test is failing, not sure why.
I have a set of conceptual question:
- The different attributes are now accessed as properties (for instance region_id). This looks cleaner, but doesn't it limit the possibility of adding custom attributes? (Technically, no, because I suppose you can just add new columns to data, but then access become inconsistent). Maybe this isn't a problem.
- Similarly, setting
region_idor other attributes is not possible after instantiation (or only via exp.data["region_id"] = new_value not consistent)
CHANGELOG.md
Outdated
| @@ -23,6 +37,8 @@ Code freeze date: YYYY-MM-DD | |||
| - Remove content tables and make minor improvements (fix typos and readability) in | |||
| CLIMADA tutorials. [#872](https://github.com/CLIMADA-project/climada_python/pull/872) | |||
| - Centroids complete overhaul. Most function should be backward compatible. Internal data is stored in a geodataframe attribute. Raster are now stored as points, and the meta attribute is removed. Several methds were deprecated or removed. [#787](https://github.com/CLIMADA-project/climada_python/pull/787) | |||
| - Exposures complete overhaul. Notably the _geometry_ column of the inherent `GeoDataFrame` is set up at initialization, while | |||
| latitude and longitude column are no longer persent there (the according arrays can be retrieved as properties of the Exposures object: `exp.latitude` instead of `exp.gdf.latitude.values`). | |||
There was a problem hiding this comment.
| latitude and longitude column are no longer persent there (the according arrays can be retrieved as properties of the Exposures object: `exp.latitude` instead of `exp.gdf.latitude.values`). | |
| latitude and longitude column are no longer present there (the according arrays can be retrieved as properties of the Exposures object: `exp.latitude` instead of `exp.gdf["latitude"].values`). |
| self.assertEqual(exp.gdf.shape[0], len(exp.gdf[INDICATOR_CENTR + 'FL'])) | ||
| np.testing.assert_array_equal(exp.gdf[INDICATOR_CENTR + 'FL'].values, expected_result) | ||
| self.assertEqual(exp.gdf.shape[0], len(exp.hazard_centroids('FL'))) | ||
| np.testing.assert_array_equal(exp.hazard_centroids('FL'), expected_result) | ||
|
|
||
| def test__init__meta_type(self): | ||
| """ Check if meta of type list raises a ValueError in __init__""" |
There was a problem hiding this comment.
| """ Check if meta of type list raises a ValueError in __init__""" | |
| """ Check if meta of type list raises a TypeError in __init__""" |
|
I realized you can actually change/add columns directly with I still have a concern if a user tries to change Maybe a placeholder setter explaining that you can actually change it through |
as u_coord.set_df_geometry_points does not use a scheduler anymore
Actually I'd prefer users to But you're right, setter methods for 'region_id', 'category_id' and the other properties are something we should consider. I'd like to delegate this to another PR though. |
# Conflicts: # CHANGELOG.md
| "| Column | Data Type | Description | Meaningful in |\n", | ||
| "| :-------------------- | :------------ | :------------------------------------------------------------------------------------- | - |\n", | ||
| "| `geometry` | Point | the geometry column of the `GeoDataFrame`, i.e., latitude (y) and longitude (x) | centroids assignment |\n", | ||
| "| `value` | float | a value for each exposure | impact calculation |\n", | ||
| "| `impf_*` | int | impact functions ids for hazard types.<br>important attribute, since it relates the exposures to the hazard by specifying the impf_act functions.<br>Ideally it should be set to the specific hazard (e.g. `impf_TC`) so that different hazards can be set<br>in the same Exposures (e.g. `impf_TC` and `impf_FL`). | impact calculation |\n", | ||
| "| `centr_*` | int | centroids index for hazard type.<br>There might be different hazards defined: centr_TC, centr_FL, ...<br>Computed in method `assign_centroids()` | impact calculation |\n", | ||
| "| `deductible` | float | deductible value for each exposure. <br>Used for insurance | impact calculation |\n", | ||
| "| `cover` | float | cover value for each exposure. <br>Used for insurance | impact calculation |\n", | ||
| "| `region_id` | int | region id (e.g. country ISO code) for each exposure | aggregation |\n", | ||
| "| `category_id` | int | category id (e.g. building code) for each exposure | aggregation |" |
There was a problem hiding this comment.
Are all these columns required now? If not, maybe you could detail which one are optional.
in order to avoid circular imports
|
many thanks for the review @spjuhel ! 🙌 |
Changes proposed in this PR:
exp.meta)np.array) of the impact function and the assigned centroids by hazard typeThis PR fixes #
PR Author Checklist
develop)PR Reviewer Checklist