Make `Impact.impact_at_reg` support all zero impacts by peanutfun · Pull Request #773 · CLIMADA-project/climada_python · GitHub
Skip to content

Make Impact.impact_at_reg support all zero impacts#773

Merged
peanutfun merged 2 commits into
developfrom
bugfix/impact-at-reg
Aug 21, 2023
Merged

Make Impact.impact_at_reg support all zero impacts#773
peanutfun merged 2 commits into
developfrom
bugfix/impact-at-reg

Conversation

@peanutfun

@peanutfun peanutfun commented Aug 18, 2023

Copy link
Copy Markdown
Member

Changes proposed in this PR:

  • Detect if the shape product of the impact matrix is zero instead of the nonzero entries. The latter could also be the case for a correctly-shaped matrix whose values are all zero.

PR Author Checklist

PR Reviewer Checklist

@peanutfun peanutfun requested a review from aleeciu August 18, 2023 09:12

@timschmi95 timschmi95 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.

The code looks good to me. I'm wondering if there is a use case to calculate imp_at_reg if all the values are zero? If not, one could also throw a different error (or warning) saying that all impacts are zero if self.imp_mat.nnz == 0 .

@peanutfun

Copy link
Copy Markdown
Member Author

@timschmi95

Copy link
Copy Markdown
Collaborator

Ah yes, calibration is a good example where all impacts can be zero. And I guess if user has 0 impacts and calculates their 0 impacts for each region it doesn't neccesarily require a warning, although it's not a very interesting calcuculation^^
From my point of view you could merge it then.

@peanutfun

Copy link
Copy Markdown
Member Author

@peanutfun peanutfun merged commit ef410e1 into develop Aug 21, 2023
@emanuel-schmid emanuel-schmid deleted the bugfix/impact-at-reg branch September 7, 2023 13:27
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