Change test data impf.id to test non-default params by chahank · Pull Request #676 · CLIMADA-project/climada_python

Conversation

@chahank

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

PR Reviewer Checklist

Chahan Kropf added 2 commits

March 28, 2023 10:14

@Evelyn-M

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)

@Evelyn-M

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?

@chahank

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())

@chahank

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.

@Evelyn-M

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 Evelyn-M deleted the bugfix/lines_poly_impf branch

March 28, 2023 09:31

@Evelyn-M Evelyn-M restored the bugfix/lines_poly_impf branch

March 28, 2023 09:34

2 participants

@chahank @Evelyn-M