Fixes possible incorrect mapping between eai metric values and exposure points in trajectories - #1297
spjuhel wants to merge 2 commits into
Conversation
Sorry, something went wrong.
peanutfun
left a comment
There was a problem hiding this comment.
Looks reasonable. But are there no tests for the results?
Sorry, something went wrong.
|
There are, just not for the very specific case where the index of the geodataframe of the exposure is not a continuous range of number. |
Sorry, something went wrong.
|
Is that a hassle to implement? Simply dropping a row from a built exposure object before the test should do it, right? |
Sorry, something went wrong.
chahank
left a comment
There was a problem hiding this comment.
Does this need a test to avoid breaking it in the future?
Sorry, something went wrong.
emanuel-schmid
left a comment
There was a problem hiding this comment.
looks nice - but sorry, I don't get it. 😕 A test would be awesome. But also a more explanatory description could help.
Sorry, something went wrong.
| index=self._date_idx, | ||
| columns=self.snapshots[0].exposure.gdf.index, |
There was a problem hiding this comment.
tbh, i don't quite understand the idea behind having both, self._date_idx and self.date_idx, especially in the combination with the @data_idx.setter - nevertheless: why not index=self.date_idx?,
Sorry, something went wrong.
There was a problem hiding this comment.
the only relevant difference to the develop branch seems to be the explicit setting of columns. however, there is no hint to the rationale in the PR description.
Sorry, something went wrong.
This PR fixes a bug in calc_eai_gdf (both for static and interpolated trajectories) where the exposure points coordinates are incorrectly mapped to the corresponding risk value when the exposure GeoDataFrame index is discontinuous.
The suggested fix directly uses the index, instead of creating it from a range.
PR Author Checklist
PR Reviewer Checklist