Remove Tag from Exposures #756
Conversation
Overriding plot_scatter and plot_hexbin to keep custom plot titles unchanged
emanuel-schmid
changed the title
crop_production, black_marble and spam_agrar also depend on it
# Conflicts: # climada/entity/exposures/base.py
# Conflicts: # CHANGELOG.md
climada/entity/exposures/base.py
Outdated
| title = (self.description.strip() if isinstance(self.description, str) else "" | ||
| ) if title is None else title |
There was a problem hiding this comment.
title = something if title is None else title seems redundant to me. Also: What other type than str would description support? Should we maybe cast it to str in the constructor to avoid testing its type here?
| title = (self.description.strip() if isinstance(self.description, str) else "" | |
| ) if title is None else title | |
| if title is None: | |
| title = self.description.strip() |
There was a problem hiding this comment.
None is the default. So if you call it with a title-argument - fine, up to you, even if it's an empty string.
If you don't bother - also fine, then the title from the description is plotted.
There was a problem hiding this comment.
your suggestion doesn't work if the description is None. You would need another if block. From there on it's a question of taste I guess.
There was a problem hiding this comment.
Then let's keep it as simple as can be: Users need to take care of formatting their descriptions. So I suggest we just do:
| title = (self.description.strip() if isinstance(self.description, str) else "" | |
| ) if title is None else title | |
| if title is None: | |
| title = self.description |
This should also work when description is None, ax.set_title(None) should have no (negative) effect.
There was a problem hiding this comment.
some util.plot function doesn't like None. Hence I did title = self.description or "".
climada/entity/exposures/base.py
Outdated
| title = (self.description.strip() if isinstance(self.description, str) else "" | ||
| ) if title is None else title |
There was a problem hiding this comment.
Same as above. title is already assigned to title 😉
climada/engine/forecast.py
Outdated
| if not ( | ||
| isinstance(decision_dict["probability_aggregation"], float) | ||
| & isinstance(decision_dict["area_aggregation"], float) | ||
| ): | ||
| ValueError( | ||
| " If decision_level is 'exposure_point'," | ||
| + "parameters probability_aggregation and " | ||
| + "area_aggregation of " | ||
| + "Forecast.plot_warn_map() must both be " | ||
| + "floats between [0..1]. Which each " | ||
| + "specify quantiles." | ||
| ) |
There was a problem hiding this comment.
How is this related to the Exposures tag?
There was a problem hiding this comment.
Not. It's just dead code that has been removed for cosmetic reasons. 🤔 Perhaps better to atctually raise the Error instead?
There was a problem hiding this comment.
Forecast will be overhauled anyway. I thinks it's best not to touch that class now. Can you revert the changes, even though the error clearly should be raised? 😅
# Conflicts: # CHANGELOG.md
Changes proposed in this PR:
description, which is used inplot_hexbinandplot_scatterto set the titleThis PR fixes #
PR Author Checklist
develop)PR Reviewer Checklist