Skip to content

Fixes possible incorrect mapping between eai metric values and exposure points in trajectories - #1297

Open
spjuhel wants to merge 2 commits into
developfrom
fix-trajectory_eai_metric
Open

spjuhel wants to merge 2 commits into
developfrom
fix-trajectory_eai_metric

Conversation

@spjuhel

@spjuhel spjuhel commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

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

@peanutfun peanutfun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable. But are there no tests for the results?

@spjuhel

spjuhel commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator Author

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.

@peanutfun

Copy link
Copy Markdown
Member

Is that a hassle to implement? Simply dropping a row from a built exposure object before the test should do it, right?

@chahank chahank left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need a test to avoid breaking it in the future?

@emanuel-schmid emanuel-schmid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks nice - but sorry, I don't get it. 😕 A test would be awesome. But also a more explanatory description could help.

Comment on lines +260 to +261
index=self._date_idx,
columns=self.snapshots[0].exposure.gdf.index,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?,

@emanuel-schmid emanuel-schmid Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

4 participants