Skip to content

Restructuring of discounting#73

Merged
JulStraus merged 4 commits intomainfrom
js/restructure_discount
Oct 2, 2025
Merged

Restructuring of discounting#73
JulStraus merged 4 commits intomainfrom
js/restructure_discount

Conversation

@JulStraus
Copy link
Copy Markdown
Collaborator

PR #71 introduced a new discounting method. Within the PR we identified potential for improving the overall code structure. This is now done within this PR. It can still be improved, e.g., providing types for the different discounting methods through different Discounter types, if desired.

I furthermore did a change to API structure which is in practice rather miniscule.

@JulStraus JulStraus requested review from hellemo and trulsf October 1, 2025 10:13
@JulStraus JulStraus added the enhancement New feature or request label Oct 1, 2025
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 1, 2025

Codecov Report

❌ Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/discount.jl 96.96% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/discount.jl 98.36% <96.96%> (+7.13%) ⬆️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JulStraus
Copy link
Copy Markdown
Collaborator Author

I updated the identification of strategic periods with a new method which also throws errors if a wrong value is specified. One issue in this case is that with the current method, we cannot say with a 100 % certainty that the TimePeriod is actually within the TimeStructure as we only compare the sp value, i.e., comparing Ints.

We can, if desired, extend the overall approach as well as the identification of a strategic period is in practice potentially something of general relevance. One issue at the moment is that is rather inefficient when discount or objective_value are used on OperationalPeriod level or comparable.

@JulStraus
Copy link
Copy Markdown
Collaborator Author

I identified one approach for a proper identification:

filter_sp(sp::AbstractStrategicPeriod, t::TimePeriod) = t in sp
filter_sp(sp::AbstractStrategicPeriod, t::AbstractOperationalScenario) = t in opscenarios(sp)
filter_sp(sp::AbstractStrategicPeriod, t::AbstractRepresentativePeriod) = t in repr_periods(sp)

function _sp_period(t, ts::TimeStructure)
    sps = collect(strat_periods(ts))
    filter_fun(sp) = filter_sp(sp, t)
    per = findfirst(filter_fun, sps)
    isnothing(per) && throw(ErrorException("Time period not part of any strategic period"))
    return sps[per]
end

This approach identifies whether the node is exactly in the time structure compared to just checking that the sp (and branch) are correct. Note that the system also works with TwoLevelTree.

Copy link
Copy Markdown
Member

@trulsf trulsf left a comment

Choose a reason for hiding this comment

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

I like the simplifications and doc updates. Currently, I can live with the version that identifies strategic period by index.

@JulStraus JulStraus merged commit ef54374 into main Oct 2, 2025
6 checks passed
@JulStraus JulStraus deleted the js/restructure_discount branch October 2, 2025 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants