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

Change test data impf.id to test non-default params #676

Merged
merged 3 commits into from
Mar 28, 2023

Conversation

chahank
Copy link
Member

@chahank chahank commented Mar 20, 2023

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

@chahank chahank requested a review from Evelyn-M March 20, 2023 17:08
@Evelyn-M
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Member Author

chahank commented Mar 28, 2023

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
Copy link
Member Author

chahank commented Mar 28, 2023

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
Copy link
Collaborator

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 merged commit e51f92d into develop Mar 28, 2023
@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
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.

2 participants