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

Make Impact.impact_at_reg support all zero impacts #773

Merged
merged 2 commits into from
Aug 21, 2023

Conversation

peanutfun
Copy link
Member

@peanutfun peanutfun commented Aug 18, 2023

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

@timschmi95 timschmi95 left a comment

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

When calibrating impact functions (see #692), one usually aggregates impacts in space, and impact_at_reg is the "canonical" way of doing that. When the optimization algorithm explores possible parameter sets, it might happen that the computed impact is zero everywhere. However, the calibration cannot recover from the error thrown in this case and aborts.

One has to distinguish two cases.

  • If save_mat=False, impact_at_reg cannot work, so I think throwing an error is fine. However, there is no way of knowing that for sure from the Impact object. By default, the impact matrix should be empty with shape (0, 0).
  • If save_mat=True, it might very well be that all values are zero and computing the spatial aggregate will also return zero. However, this should not be an error (currently, it is), and I also don't see why this should issue a warning. It's simply the arithmetic result.

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

Thanks for the review and discussion, @timschmi95!

@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