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

Improve impact calc error messages #691

Merged
merged 6 commits into from
Apr 17, 2023

Conversation

leonie-villiger
Copy link
Collaborator

@leonie-villiger leonie-villiger commented Apr 5, 2023

Changes proposed in this PR:
ImpactCalc.impact(exposures, impfset, hazard) checks that

  • exposures refers to impact functions for the given hazard type
  • impfset contains impact functions for the given hazard type.
    In case of mismatching hazard types, a comprehensible error message is produced. Currently, these cases result in the cryptic error message AttributeError: 'list' has no attribute 'calc_mdr'

This PR fixes #669

PR Author Checklist

PR Reviewer Checklist

@chahank
Copy link
Member

chahank commented Apr 6, 2023

Great solution to issue #669 ! Thanks! Just a few suggestion to make the test more efficient and clearer.

@ChrisFairless
Copy link
Collaborator

I ran into this error today. An hour later I'm here and very pleased to see this fix ✨

@leonie-villiger
Copy link
Collaborator Author

Thank you for the review! I think I addressed all your comments. Please merge the branch If you agree.

@leonie-villiger leonie-villiger requested a review from chahank April 17, 2023 06:27
@chahank
Copy link
Member

chahank commented Apr 17, 2023

Great, thanks! Ready to merge imho.

@chahank
Copy link
Member

chahank commented Apr 17, 2023

Welcome to the authors list!

@leonie-villiger leonie-villiger merged commit c7bcf32 into develop Apr 17, 2023
@leonie-villiger leonie-villiger deleted the feature/improve_impact_calc_error_messages branch April 20, 2023 06:08
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.

3 participants