Conversation
- altered impact plot titles
- altered impact plot titles
as it is not expected to be used outside of engine.impact
make IFC a data class
add missing checks in test_impact and hazard.test_base
climada/hazard/base.py
Outdated
| LOGGER.warning("resetting the frequency on a hazard object who's frequency unit" | ||
| "is %s and not %s will most likely lead to unexpected results", |
There was a problem hiding this comment.
I do not really like the "will lead to unexpected results". This very vague. Can we make it more precise you think?
There was a problem hiding this comment.
Good question! Can we? The thing is that the user is sending contradicting signals at this point. Why do they set a frequency_unit other than 'annual' and then still use the method in a way that is strongly related to 'annual' frequencies.
I'd say they have to look and see themselves. 🤷
There was a problem hiding this comment.
I think we could say something like:
"The frequency unit will be changed from %s to %s. The resulting frequency will thus be most likely incorrect. Consider setting reset_frequency=False and set the frequency manually for the selection."
| "| coord_exp |np.array| exposures coordinates [lat, lon] (in degrees) (Exposure.gdf.latidues, Exposure.gdf.longitude)|\n", | ||
| "| frequency |np.array| annual frequency of events (Hazard.frequency)|\n", | ||
| "| frequency |np.array| frequency of events (Hazard.frequency)|\n", | ||
| "| frequency_unit |str| unit of event frequency, by default '1/year', i.e., annual, this attribute is purely informative and does not effect calculations (Hazard.frequency_unit)|\n", |
There was a problem hiding this comment.
Is it really needed to the "this attribute is purely informative..." ? For instance, this is also the case for the event_id, the unit or the date. And this can change in the future, in particular if time is integrated more deeply into CLIMADA.
There was a problem hiding this comment.
Just wanted to emphasize that the frequency_unit is not being considered at any point in calculations. All it really does is change labels in plots.
As a naive user, when I can set a frequency_unit in an object and I get a frequency, I might naturally suspect the two to be somehow correlated. Which is not the case. Therefore the warning.
There was a problem hiding this comment.
But this is also the case for other entries. Thus, I would remove it as it is confusing why this is said for that entry, but not for others. Furthermore, there will likely be methods in the future that will actually care about the frequency unit.
|
Thanks for the changes! Very useful. A few small comments. |
|
@chahank Thanks for reviewing! 🎉 |
…limada_python into feature/frequency_unit impact tutorial: change frequency_unit description
2 participants
Changes proposed in this PR:
frequency_unitin classesImpact,ImpactFreqCurveThe fields have no impact on the calculation whatsoever, they are purely informative.
Nevertheless concatenation of
Hazardobjects is inhibited if they have differentfrequency_unitvalues.This PR fixes issue #516
Pull Request Checklist
develop)