[Option Appraisal Module] [Package 1: Measures] Split 3: Adds new base Measure class#1279
[Option Appraisal Module] [Package 1: Measures] Split 3: Adds new base Measure class#1279spjuhel wants to merge 69 commits into
Measure class#1279Conversation
…raisal-costincome
…on-appraisal-costincome
Cleans-up, Docstringfies Better to_dict, color parser, and post_inits Removes duplicate docstring
…MADA-project/climada_python into feature/option-appraisal-dataclasses
…on-appraisal-costincome
…e/option-appraisal-dataclasses
…on-appraisal-costincome
…n-appraisal-new-measure-base
Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>
…e/option-appraisal-dataclasses
…on-appraisal-costincome
…n-appraisal-new-measure-base
…raisal-new-measure-base
…raisal-new-measure-base
ValentinGebhart
left a comment
ValentinGebhart
left a comment
There was a problem hiding this comment.
This new implementation of the measures looks super cool :) I think it is much cleaner and user-friendly to before! I just commented whenever something was unclear to me. I haven't had time to check the working of the functions with actual data yet , e.g. for some consistency checks.
The only one more conceptual question I have is if you have to work with the (hidden) base_exposure, base_hazard, base_impfunc handling, but maybe I have not understood yet why these can be useful. Happy to discuss!
Note that I first read the tutorial without knowing the code to just check if I understand the usage. Some of the comments there might not be there know after I read the code :)
| "\n", | ||
| "```python\n", | ||
| "new_exp, new_impf, new_haz = retrofitting.apply(exp, impf, haz, kwargs_hazard={'threshold': 3.5})\n", | ||
| "impact_with_measure = measure.calc_impact(exp, impf, haz)\n", |
There was a problem hiding this comment.
This lines is not clear to me. Shouldn't it be
impact_with_measure = retrofitting.calc_impact(exp, impf, haz)?
Also, do I need the first line before running the second one? Or are they two separate things you can to with the measure object? It might make sense to clarify with to brief descriptions?
There was a problem hiding this comment.
Yup, that's just plain wrong! Will rework this.
| "By default, any component not explicitly defined uses the `identity_function`, which simply returns the input object without modification.\n", | ||
| "```\n", | ||
| "\n", | ||
| "The **only** requirement for these python functions is to **take** and **return** an `Exposures` object (or `ImpactFuncSet` or `Hazard`), which enable advanced modeling.\n", |
There was a problem hiding this comment.
I suggest to make these two sentences a bit clearer. Eg., you do not say what base_exposures, base_impfset, base_hazard are. One suggestion:
"
The only requirement for these python functions is to take and return an Exposures object (or ImpactFuncSet / Hazard objects, respectively), a structure that enables full modeling flexibility.
Notably, the apply() method always passes the entire input triplet of exposure, impact function set, and hazard to every transformation function, so you define measures based on interactions between all three components.
"
| "import numpy as np\n", | ||
| "\n", | ||
| "\n", | ||
| "def reduce_exposure_by_depth(exposures, reduction_factor=0.2, threshold=2.0, **kwargs):\n", |
There was a problem hiding this comment.
by only reading this tutorial and knowing that in the back you also also hand in base_exposures, base_impfset, and base_hazard to all functions, I am just wondering if this is not a bit confusing. E.g., could the exposures input be different to the base_exposures input? What would that mean? (Note that below you actually note this, but it might be nice to give an example for a case where this is necessary/useful).
Just a naive thought without looking at the code:
Couldn't each of the three functions take all (exp, impfset, haz) as input, and no hidden base objects? e.g. exposures_changes is always a function that has these three inputs. Anyways if it is usefull to have an expsosure input but also a hidden base_exposure, disregard this suggestion.
| " kwargs_exposures={'reduction_factor': 0.5, 'threshold': 2.0}\n", | ||
| ")\n", | ||
| "\n", | ||
| "# You can also overide this with another \"base\" hazard in the \n", |
There was a problem hiding this comment.
see comment above: can you give a simple example about when this is necessary or useful?
| " kwargs_exposures={'reduction_factor': 0.5, 'threshold': 2.0}\n", | ||
| ")\n", | ||
| "\n", | ||
| "# You can also overide this with another \"base\" hazard in the \n", |
There was a problem hiding this comment.
see comment above: can you give a simple example about when this is necessary or useful?
|
|
||
| Notes | ||
| ----- | ||
| The kwargs merging follows this priority order: |
There was a problem hiding this comment.
maybe not super clear. In my mind prio 1 is overriding prio 2, but below I think 2. is overriding 1.? maybe rephrase
| """Compute impact with the active measure. | ||
|
|
||
| This convenience method allow to compute an ``Impact`` object from an | ||
| (Exposure, ImpactFuncSet, Hazard) triplet by first applying the measure |
There was a problem hiding this comment.
"after having applied" instead of "by first applying"?
There was a problem hiding this comment.
| (Exposure, ImpactFuncSet, Hazard) triplet by first applying the measure | |
| (Exposure, ImpactFuncSet, Hazard) triplet after having applied the measure |
| ) | ||
| new_exp.assign_centroids(new_haz) | ||
| imp = ImpactCalc(new_exp, new_impfs, new_haz).impact( | ||
| save_mat=False, assign_centroids=False |
There was a problem hiding this comment.
why do we need save_mat False hard-coded? Could this not be an optional parameter?
| List of measure names that this measure is a combination of. | ||
| cost_income : climada.entity.measures.cost_income.CostIncome | ||
| Cost and income object associated with the measure. | ||
| implementation_duration : pd.DateOffset, optional |
There was a problem hiding this comment.
as the sub_measures attribute, the implementation_duration attribute is not used anywhere directly? is this here for later additions to the code?
There was a problem hiding this comment.
Yes, I should just remove it for this PR.
| impfset_changes=track("impfset"), | ||
| ) | ||
| m.apply(_make_mock(), _make_mock(), _make_mock(), enforce_copy=False) | ||
| assert call_order == ["exposures", "hazard", "impfset"] |
There was a problem hiding this comment.
very cool tests with all the mocking :)
I'm wondering if it makes sense to eventually also add some tests that actually check numbers, e.g. when you combine two different effects (e.g. using the "base" objects) to see if the effect is what you want? Or should the mock tests above be enough to see that the effects of the change functions (and their interactions?) are the intended ones? Sorry for this vague comment ;)
There was a problem hiding this comment.
My understanding is that would be integration tests (climada/test/test_measures.py).
I'm not sure if they are satisfying yet though.
Thanks for these comments. Answering the conceptual question about the The idea comes from the fact that I assume there is a whole range of adaptation options effect on But it is a bit "advanced" to write for instance (ellipsed/simplified): def my_effect_on_exposures(exposures, req_hazard):
indices = req_hazard > 10
exposures.gdf.loc[indices,"value"] *= 0.98
measure_a = Measure(..., exposure_change=my_effect_on_exposures)
exp, impf, haz = measure.apply(exp, impf, haz, kwarg_exposures = {req_hazard: haz}) # That part is slightly scaryBy always providing def my_effect_on_exposures(exposures, base_hazard): # note that the name need to be base_hazard!
indices = base_hazard > 10
exposures.gdf.loc[indices,"value"] *= 0.98
measure_a = Measure(..., exposure_change=my_effect_on_exposures)
exp, impf, haz = measure.apply(exp, impf, haz) # Scary kwargs are gone!On this small example it may seem not much, but if the three effects all rely on the three components then it is way more relevant. I agree the current explanations in the docs might be improved ;D |
| A ``Measure`` represents a single adaptation or risk-reduction action. It | ||
| holds three (optional) transformation functions, one each for | ||
| :class:`Exposures`, :class:`ImpactFuncSet`, and :class:`Hazard`, that are | ||
| can be applied to a triplet of ``(Exposures, ImpactFuncSet, Hazard)``, to |
There was a problem hiding this comment.
| A ``Measure`` represents a single adaptation or risk-reduction action. It | |
| holds three (optional) transformation functions, one each for | |
| :class:`Exposures`, :class:`ImpactFuncSet`, and :class:`Hazard`, that are | |
| can be applied to a triplet of ``(Exposures, ImpactFuncSet, Hazard)``, to | |
| A ``Measure`` represents an adaptation or risk-reduction action | |
| (or a bundle of actions considered as one). It holds three (optional) | |
| transformation functions, one each for :class:`Exposures`, | |
| :class:`ImpactFuncSet`, and :class:`Hazard`, that are | |
| can be applied to a triplet of ``(Exposures, ImpactFuncSet, Hazard)``, to |
| The kwargs merging follows this priority order: | ||
|
|
||
| 1. Default context: The original triplet (exposures, impfset, hazard) | ||
| 2. Entity-specific overrides: Values from ``kwargs_exposures``, | ||
| ``kwargs_impfset``, or ``kwargs_hazard`` respectively |
There was a problem hiding this comment.
| The kwargs merging follows this priority order: | |
| 1. Default context: The original triplet (exposures, impfset, hazard) | |
| 2. Entity-specific overrides: Values from ``kwargs_exposures``, | |
| ``kwargs_impfset``, or ``kwargs_hazard`` respectively | |
| Keyword arguments other than "kwargs_exposures", "kwargs_impfset", | |
| "kwargs_hazard" are ignored. | |
| The base exposure, hazard, and impfset provided by default can | |
| be overriden by explicitely providing a different value within | |
| ``kwargs_exposures``, ``kwargs_impfset``, or ``kwargs_hazard`` | |
| respectively. |
| imp = ImpactCalc(new_exp, new_impfs, new_haz).impact( | ||
| save_mat=False, assign_centroids=False | ||
| ) |
There was a problem hiding this comment.
| imp = ImpactCalc(new_exp, new_impfs, new_haz).impact( | |
| save_mat=False, assign_centroids=False | |
| ) | |
| imp = ImpactCalc(new_exp, new_impfs, new_haz).impact( | |
| save_mat=kwargs.get("save_mat",False), assign_centroids=kwargs.get("assign_centroids",False) | |
| ) |
| return changed_exposures, changed_impfset, changed_hazard | ||
|
|
||
| def calc_impact( | ||
| self, exposures: Exposures, impfset: ImpactFuncSet, hazard: Hazard |
There was a problem hiding this comment.
| self, exposures: Exposures, impfset: ImpactFuncSet, hazard: Hazard | |
| self, exposures: Exposures, impfset: ImpactFuncSet, hazard: Hazard, **kwargs: dict, |
| hazard : Hazard | ||
| The initial (no adaptation) hazard set | ||
|
|
||
| Returns |
There was a problem hiding this comment.
| hazard : Hazard | |
| The initial (no adaptation) hazard set | |
| Returns | |
| hazard : Hazard | |
| The initial (no adaptation) hazard set | |
| kwargs: dict | |
| Keyword arguments passed to the `impact()` method of `ImpactCalc` | |
| (Currently `save_mat` and `assign_centroids`) | |
| Returns |
2 participants
Changes proposed in this PR:
This PR brings the basis of the refactored
Measureclass, using closures (functions that return a function) to define the effects of a measure.Important points:
Exposures, effects onImpfset, and onHazardallow_kwargsto make sure closures always accept kwargs.Measureinstances optionally have a_configattribute that contains aMeasureConfigif the measure was defined from such an object (and is therefore serializable).PR Author Checklist
develop)PR Reviewer Checklist