Enhance discount_avg function to support resolution parameter#71
Enhance discount_avg function to support resolution parameter#71
Conversation
JulStraus
left a comment
There was a problem hiding this comment.
I can see the appeal in adding additional methods for the discounting. However, there are a few thinks which I am a bit confused about:
- We do not allow other resolutions compared to 1. In this situation it is a binary decision. Should we extend it to allow for also other resolutions? That is rather straight forward through adding it as keyword argument. If not, we can also just use a Bool instead of Int as input.
- It is unclear from the documentation/docstrings what keyword arguments are allowed for
type. We should add that. As a user is normally utilizing the functionobjective_weight, it would be necessary to add it there as well and not only indiscount. - The test is testing that the old
avgis smaller, but we do not have any tests showing that it is correctly calculated?
Generally speaking, I think we could also restructure the discounting within TimeStruct. It is currently not entirely clear what is allowed for which input, at least in my opinion. Whether that should be part of this PR or a separate is up to debate. I could take a look at it.
As an example, we do not allow for discounting on OperationalScenario or RepresentativePeriod while we do allow for OperationalPeriod even if it is just a fallback for the strategic period. This is at least in my opinion not entirely consistent as it should be possible to also utilize discounting then on the other types.
|
I did a bit of rewrite based on the comments and have (hopefully) improved the docs and tests a bit. I agree that the whole discounting could be up for a better design and further discussions. I suggest to move that to a separate PR, as I would like to make a release with support for the new yearly average discounting. If you would have a go of such a PR, you are more than welcome to have a go at it. |
JulStraus
left a comment
There was a problem hiding this comment.
I think it looks good right now. The restructuring removed actually one if loop :)
I will take a look at it regarding restructuring it today. So if you do not mind to wait 1 day for registering it, we can take it in the same new version.
Working with OpenEMPIRE code, there was a need to support additional discount calculations. I am not totally happy with using 0 as a default value for the
resolutionparameter to indicate a continuous calculation (I would prefer Inf, but that would mean having to convert from a float somewhere).