Change test data impf.id to test non-default params by chahank · Pull Request #676 · CLIMADA-project/climada_python
Conversation
Changes proposed in this PR:
This PR does NOT provide a bugfix after all, as this was fixed by chance in PR #666 . However, the tests do not test for this case, and thus it was added to the tests. It is very minimalistic and could be made even more consistent. But at least it provides a minimal improvement.
Basically, the impact function id is by default set to 1 under the hood in CLIMADA if none is specified. Currently, the tests only work with impact function id 1. Thus, the use case when multiple impact functions with different ids are used is not tested. The default exposures for polygons was now set to use impact functions with id = 2.
PR Author Checklist
- Read the Contribution Guide
- Correct target branch selected (if unsure, select
develop) - Descriptive pull request title added
- Source branch up-to-date with target branch
- Documentation updated
- Tests updated
- Tests passing
- No new linter issues
- Changelog updated
PR Reviewer Checklist
- Read the Contribution Guide
- CLIMADA Reviewer Checklist passed
- Tests passing
- No new linter issues
Chahan Kropf added 2 commits
March 28, 2023 10:14I'm not sure how the tests really checks for this case now. The two impact functions in the currently tested impact function set are functionally the same (the welker ones), and i cannot see how it is really ensured that not the imp func with ID 1 is taken (as the impact values would be the same)
aka, would it make sense to explicitly check that the interpolated exp_poly still has imp func id 2? and perhaps with a different impact function altogether?
aka, would it make sense to explicitly check that the interpolated exp_poly still has imp func id 2? and perhaps with a different impact function altogether?
This is exactly what is done in all the tests:
def check_unchanged_geom_gdf(self, gdf_geom, gdf_pnt):
"""Test properties that should not change"""
for n in gdf_pnt.index.levels[1]:
sub_gdf_pnt = gdf_pnt.xs(n, level=1)
rows_sel = sub_gdf_pnt.index.to_numpy()
sub_gdf = gdf_geom.loc[rows_sel]
self.assertTrue(np.alltrue(sub_gdf.geometry.geom_equals(sub_gdf_pnt.geometry_orig)))
for col in gdf_pnt.columns:
if col not in COL_CHANGING:
np.testing.assert_allclose(gdf_pnt[col].unique(), gdf_geom[col].unique())
I'm not sure how the tests really checks for this case now. The two impact functions in the currently tested impact function set are functionally the same (the welker ones), and i cannot see how it is really ensured that not the imp func with ID 1 is taken (as the impact values would be the same)
That is correct.
aka, would it make sense to explicitly check that the interpolated exp_poly still has imp func id 2? and perhaps with a different impact function altogether?
This is exactly what is done in all the tests:
def check_unchanged_geom_gdf(self, gdf_geom, gdf_pnt): """Test properties that should not change""" for n in gdf_pnt.index.levels[1]: sub_gdf_pnt = gdf_pnt.xs(n, level=1) rows_sel = sub_gdf_pnt.index.to_numpy() sub_gdf = gdf_geom.loc[rows_sel] self.assertTrue(np.alltrue(sub_gdf.geometry.geom_equals(sub_gdf_pnt.geometry_orig))) for col in gdf_pnt.columns: if col not in COL_CHANGING: np.testing.assert_allclose(gdf_pnt[col].unique(), gdf_geom[col].unique())
ah yeah ok, that was a bit cryptic to spot.
looks good, then.
Evelyn-M
deleted the
bugfix/lines_poly_impf
branch
Evelyn-M
restored the
bugfix/lines_poly_impf
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters