Hotfix: assign centroids in cost benefit impact calculation if in new exposure no centroids were assigned#1066
Conversation
ValentinGebhart
requested review from
chahank,
emanuel-schmid and
peanutfun
as code owners
|
@spjuhel FYI; but I guess not relevant for the cost benefit restructuring |
peanutfun
left a comment
peanutfun
left a comment
There was a problem hiding this comment.
Thank you for the fix!
I would appreciate a test. Could you just add the minimal example from the issue as test case? The test should make sure that the computation runs even when no centroids are assigned, and that the warning is issued. Alternatively, it might also be easy to add such checks to the existing unit tests for Measures and CostBenefit.
Co-authored-by: Lukas Riedel <[email protected]>
|
Thanks, Lukas! I've added a small test that check that the calculation runs if no centroids are assigned and that a warning is raised, and that no warning is raised if centroids are already assigned. |
|
@peanutfun I have know changed the case when we check that the centroid warning should NOT be raised, as we discussed. I checked that it works (ie raises an error if the centroid warning is raised wrongly, and passes if no warning is raised). It somehow looks a bit awkward to me, but if you think this is good enough, we can merge. |
2 participants
Changes proposed in this PR:
This PR fixes #1056
PR Author Checklist
develop)PR Reviewer Checklist