[Option Appraisal Module] [Package 1: Measures] Split 2: Adds CostIncome class for measure financial aspects management#1277
Conversation
…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
|
This PR is officially open for review! @luseverin would you be so kind to review the tutorial (to spare @chahank who has to review a lot of my code lately) |
|
Hi @spjuhel, yes I'll be happy to review the tutorial :) |
…on-appraisal-costincome
…e/option-appraisal-dataclasses
…on-appraisal-costincome
…on-appraisal-costincome
This reverts commit 775c621.
…on-appraisal-costincome
There was a problem hiding this comment.
Cell 1:
- Do you need to specify the sign convention? I think it might be confusing as the users anyways input the costs with a positive sign.
- Maybe already define the meaning of the values Y, M, Q for the freq parameter here?
Cell 2:
- comment not matching code (20000 vs 50000 and 12000 vs 8000)
Cell 3:
- "Three methods are available to calculate cash flows:
- I would maybe repeat the parameters instead of the ... for clarity
Cell 5:
- Consider an option to add a row with totals in the dataframe format
Cell 6:
- ... draws a bar chart of the cash flows
Cell 7:
- Is it voluntary that you do not compare the same
init_cost,periodic_costandperiodic_incomebetween the with- and without-growth instances? See the comment on cell 2.
Cell 10:
- Not exactly sure of what is meant with "annualised" but it is probably because I am not familiar with the field :) I think I get the idea though
Cell 11:
- Do we care about the days when we consider the monthly case? What happens if we put 2022-01-15 instead of 2022-01-01 for the date? Maybe you could shortly comment on that (if relevant)
Cell 13:
- Maybe add an example of the manual merge of the custom flows?
Cell 15:
- The From YAML part seems to be incomplete here, but you are probably aware of it :)
There was a problem hiding this comment.
Hi @luseverin,
Thanks for the review!
Here are some of the changes I made (also based on @chahank remarks):
- Improved the Quickstart to show more of the capabilities right from the start, with minimal amount of scrolling.
- Added meaning of periods aliases as well as url to pandas documentation
- Aligned the values with comment as well as for the different examples
- Improved description of the plotting function
- Improved details on sub-yearly period frequencies
- Improved description of the merging (also now custom cashflows are merged)
I think I adressed all other comments.
For the YAML part, I don't want the notebook to have to write a file, so the code is within the markdown, not as a python cell. I added a bit more to it.
luseverin
left a comment
There was a problem hiding this comment.
Hey @spjuhel,
I just reviewed the tutorial, great work! As a new user of the module, with a limited knowledge on the topic I could understand everything, so I think it is a good sign! The module looks very neat and I look forward to experimenting it, congrats! I had only minor comments (see comments on the file). Apologises as I wrote them as a block because I could not find the option to do the line by line (or cell by cell) commenting in the notebook directly.
Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>
…e/option-appraisal-dataclasses
…on-appraisal-costincome
…raisal-costincome
|
@ValentinGebhart this one still requires code review. You can ignore the files related to MeasureConfig (they'll be reviewed in the other PR) and focus on |
ValentinGebhart
left a comment
There was a problem hiding this comment.
Thanks @spjuhel for this nice addtition and structure. It think it is well written and seems to compute what it should compute. Also well tested. I added a few comments but I think most of them are minor. Great work!
|
|
||
| Attributes | ||
| ---------- | ||
| mkt_price_year : datetime, default to today's year. |
There was a problem hiding this comment.
I think also str or int or datetime object word, right?
There was a problem hiding this comment.
from code below this should be int
| custom_cash_flows : pd.DataFrame, optional | ||
| User-defined cash flows indexed by date. | ||
| freq : str | ||
| Frequency of the cash flows (e.g., 'Y', '3M', '7D'). |
There was a problem hiding this comment.
you could add that all pandas period aliases are allowed?
| def __init__( | ||
| self, | ||
| *, | ||
| mkt_price_year: Optional[int] = None, |
There was a problem hiding this comment.
see comment above, maybe adapt docstring if only ints work here
|
|
||
| Parameters | ||
| ---------- | ||
| mkt_price_year : datetime, default to today's year. |
|
|
||
| df = None | ||
| if config.custom_cash_flows is not None: | ||
| df = pd.DataFrame(config.custom_cash_flows) |
There was a problem hiding this comment.
Just a note for compatibility with config, that you probably already are aware. Here is seems the config.custom_cash_flows must have cost and income columns, but in the config is seems only to have values which can be positive or negative:
This currently breaks (copied definition from other tutorial)
custom_schedule = [
{"date": "2024-01-01", "value": -1000000}, # Initial cost
{"date": "2029-01-01", "value": -200000}, # Mid-term overhaul
{"date": "2034-01-01", "value": 500000}, # Terminal value
]
custom_finance = CostIncomeConfig(custom_cash_flows=custom_schedule)
ci = CostIncome.from_config(custom_finance)|
|
||
| Parameters | ||
| ---------- | ||
| impl_date : |
There was a problem hiding this comment.
is it on purpose that these parameters do not have type suggestions? Because there would be too many that work?
| impl_ts = pd.Timestamp(impl_date) | ||
| periods = pd.period_range(start=start_date, end=end_date, freq=self.freq) | ||
|
|
||
| results = [self._calc_at_date(impl_ts, p.start_time) for p in periods] |
There was a problem hiding this comment.
the single-date cash flows are here always computed with the start time of the period. Is this consistent with how they are computed? Could it not happen that sometimes a cash flow is left out if it is "in the middle" of a period?
|
|
||
| Returns | ||
| ------- | ||
| plt.Figure |
There was a problem hiding this comment.
the code seems to return tuple of axis (ax_bar, ax_net), which is not the same as plt.Figure?
| return cls.from_dict(yaml.safe_load(f)["cost_income"]) | ||
|
|
||
| @classmethod | ||
| def _freq_to_days(cls, freq: str) -> str: |
There was a problem hiding this comment.
It looks to me that this private method is not used anywhere? Will it be used somewhere else?
| offset = pd.tseries.frequencies.to_offset(freq) | ||
| return float(((ref + offset) - ref).days) | ||
|
|
||
| def _calc_at_date( |
There was a problem hiding this comment.
not clear if this is oblic or private function? looks private here but public in test?
Changes proposed in this PR:
Introduces
CostIncomeclass to represent the financial aspect of adaptation measures (Measures) (essentially initial and maintenance costs and possible income)Tutorial here: https://climada-python--1277.org.readthedocs.build/en/1277/user-guide/climada_cost_income.html
PR Author Checklist
develop)PR Reviewer Checklist