Skip to content

[Option Appraisal Module] [Package 1: Measures] Split 2: Adds CostIncome class for measure financial aspects management#1277

Open
spjuhel wants to merge 48 commits intodevelopfrom
feature/option-appraisal-costincome
Open

[Option Appraisal Module] [Package 1: Measures] Split 2: Adds CostIncome class for measure financial aspects management#1277
spjuhel wants to merge 48 commits intodevelopfrom
feature/option-appraisal-costincome

Conversation

@spjuhel
Copy link
Copy Markdown
Collaborator

@spjuhel spjuhel commented Apr 7, 2026

Changes proposed in this PR:

Introduces CostIncome class 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

PR Reviewer Checklist

@spjuhel spjuhel changed the base branch from main to develop April 7, 2026 14:46
@spjuhel
Copy link
Copy Markdown
Collaborator Author

spjuhel commented Apr 10, 2026

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)
Feel free to look at the code too, but I think that part needs to be reviewed by a Code Owner in any case.

@spjuhel spjuhel marked this pull request as ready for review April 10, 2026 07:23
@luseverin
Copy link
Copy Markdown
Collaborator

Hi @spjuhel, yes I'll be happy to review the tutorial :)

Copy link
Copy Markdown
Collaborator

@luseverin luseverin Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_cost and periodic_income between 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 :)

Copy link
Copy Markdown
Collaborator

@luseverin luseverin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

2 participants