-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add functionality in tools.costs
to specify cost reduction scenarios and values
#255
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #255 +/- ##
=======================================
- Coverage 76.7% 75.8% -0.9%
=======================================
Files 203 203
Lines 15704 15733 +29
=======================================
- Hits 12050 11931 -119
- Misses 3654 3802 +148
|
3b4a9d5
to
68a1d3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good and align with the intended purpose of specifying customized cost reduction values for material module technologies. We can proceed with merging this. I will use it to implement differentiation for steel and cement technologies, which will then be merged into the ssp-dev branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, this looks mostly good to me! I would add more type hints and adjust the unit test rather than removing it, if possible :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat strange to remove tests, but we seem to cover all lines touched by the PR even without it. Is there truly no need for this unit test any longer?
Even if this function is only ever called by another function, a unit test runs faster than the whole exterior function and grants confidence when debugging the code that this particular part still works or pinpoints the failure.
If it's not too much work, I think I'd like to see the test adjusted rather than removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the test for get_cost_reduction_data()
because the function was completely removed/replaced, but I now added new tests for the two new functions: _get_module_scenarios_reduction()
and _get_module_cost_reduction()
Thanks @GamzeUnlu95 ! I removed the dummy data I created to avoid confusion when you need to use the module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, looks good to me now :)
Feel free to completely type the new tests, but no need (yet ;)
# The function runs without error | ||
result = get_cost_reduction_data(module) | ||
def test_get_module_scenarios_reduction( | ||
module: Literal["energy", "materials", "cooling"], t_exp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, even with type hints in the tests already :)
If you want to completely type this function: t_exp
seems to be a set[str, str, str]
or set[str, ...]
if you want to be more generic :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @glatterf42 ! :)
i tried to add type hints for t_exp
-- both set[str, str, str]
and set[str, ...]
bring up mypy
linting errors, but set[str]
seems to work, so hopefully that's okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, I was confusing set
and tuple
🤦♂️
Currently the DAC technologies are added to the `energy` cost submodule. With #255, there is now the functionality to add submodule-specific cost assumptions. So I am moving the DAC technologies' cost reduction categories and reduction scenarios to its own submodule directory.
Currently the DAC technologies' cost reduction and reduction scenarios assumptions are added to the `energy` module's CSV files. With #255, there is now the functionality to add module-specific cost assumptions. So I am moving the DAC technologies' cost reduction categories and reduction scenarios to its own module directory.
Currently the DAC technologies' cost reduction and reduction scenarios assumptions are added to the `energy` module's CSV files. With #255, there is now the functionality to add module-specific cost assumptions. So I am moving the DAC technologies' cost reduction categories and reduction scenarios to its own module directory.
Currently the DAC technologies' cost reduction and reduction scenarios assumptions are added to the `energy` module's CSV files. With #255, there is now the functionality to add module-specific cost assumptions. So I am moving the DAC technologies' cost reduction categories and reduction scenarios to its own module directory.
Currently the DAC technologies' cost reduction and reduction scenarios assumptions are added to the `energy` module's CSV files. With #255, there is now the functionality to add module-specific cost assumptions. So I am moving the DAC technologies' cost reduction categories and reduction scenarios to its own module directory.
This PR adds the functionality for a user to add specific cost reduction values and scenario assumptions in the costs tool.
This PR closes:
tools.costs
does not allow for module-specific cost reduction scenarios and values #251The logic is as follows (for both the cost reduction values and the reduction scenario categories):
For the time being, I have added some dummy examples using thematerials
module. The following cases I tested for:Specifying scenario assumptions and cost reduction values for a technology that only exists in thematerials
module.Specifying only scenario categories for amaterials
technology that is mapped to anenergy
technology. So while the cost reduction values associated with the categories for theenergy
technology is used, the customized scenario assumptions in thematerials
takes precedence.Specifying cost reduction values that already exists in theenergy
module with its own cost reduction values (therefore replacing the original cost reduction values).Specifying scenario assumptions and cost reduction values for a technology that does not have these values specified in theenergy
module. Therefore, if runningenergy
module, then this technology has no cost reduction (because empty). But if runningmaterials
module, then the specified assumptions and values will be used.I can remove the dummy examples after reviews say this methodology/PR makes sense.
How to review
For @GamzeUnlu95: Could you check if the methodology implemented in this PR would work for the costs differentiation you want to achieve for steel and cement technologies?
For @khaeru and/or @glatterf42 : Read the diff and note that the CI checks all pass.
PR checklist
Add or expandModify tests; coverage checks both ✅