Skip to content
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 sample_backward_noise_terms_with_state #742

Merged
merged 20 commits into from
Jun 4, 2024
Merged

Conversation

arthur-brigatto
Copy link
Contributor

Closes #735.

@arthur-brigatto arthur-brigatto marked this pull request as draft May 13, 2024 17:15
@arthur-brigatto arthur-brigatto changed the title Add the possibility of using the outgoing state in the backward sampling step Add the possibility of using the outgoing state in the backward sampling step [WIP] May 13, 2024
@arthur-brigatto arthur-brigatto marked this pull request as ready for review May 13, 2024 17:16
@andrewrosemberg
Copy link
Contributor

A simple test would be great to see if it works!

src/algorithm.jl Outdated Show resolved Hide resolved
@odow
Copy link
Owner

odow commented May 13, 2024

You can add a new test as a function just after this one:

function test_MonteCarloSampler_100()
model = SDDP.LinearPolicyGraph(
stages = 1,
lower_bound = 0.0,
direct_mode = false,
) do node, stage
@variable(node, 0 <= x <= 1)
SDDP.parameterize(node, [1, 3], [0.9, 0.1]) do ω
return JuMP.set_upper_bound(x, ω)
end
end
terms =
SDDP.sample_backward_noise_terms(SDDP.MonteCarloSampler(100), model[1])
term_count = 0
for term in terms
@test term.probability == 0.01
if term.term == model[1].noise_terms[1].term
term_count += 1
else
term_count -= 1
end
end
@test term_count > 20
return
end

@@ -112,6 +112,7 @@ abstract type AbstractBackwardSamplingScheme end
sample_backward_noise_terms(
backward_sampling_scheme::AbstractBackwardSamplingScheme,
node::Node{T},
state::Dict{Symbol,Float64},
Copy link
Owner

Choose a reason for hiding this comment

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

Okay, so:

I want two methods:

  1. the existing sample_backward_noise_terms
  2. a new sample_backward_noise_terms_with_state

as well as a fallback

function sample_backward_noise_terms_with_state(
    sampler::AbstractBackwardSamplingScheme,
    node::Node,
    state::Dict{Symbol,Float64},
)
    return sample_backward_noise_terms(sampler, node)
end

The reason for this is to maintain backwards compatibility: we want all existing code to continue to work, but we want the ability for new code to use sample_backward_noise_terms_with_state instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I had the wrong idea. I will correct it.

src/plugins/headers.jl Outdated Show resolved Hide resolved
@arthur-brigatto
Copy link
Contributor Author

You can add a new test as a function just after this one:

function test_MonteCarloSampler_100()
model = SDDP.LinearPolicyGraph(
stages = 1,
lower_bound = 0.0,
direct_mode = false,
) do node, stage
@variable(node, 0 <= x <= 1)
SDDP.parameterize(node, [1, 3], [0.9, 0.1]) do ω
return JuMP.set_upper_bound(x, ω)
end
end
terms =
SDDP.sample_backward_noise_terms(SDDP.MonteCarloSampler(100), model[1])
term_count = 0
for term in terms
@test term.probability == 0.01
if term.term == model[1].noise_terms[1].term
term_count += 1
else
term_count -= 1
end
end
@test term_count > 20
return
end

Working on it!

src/plugins/headers.jl Outdated Show resolved Hide resolved
src/plugins/headers.jl Outdated Show resolved Hide resolved
arthur-brigatto and others added 2 commits May 13, 2024 20:22
Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.44%. Comparing base (8ed2e30) to head (dff9395).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #742      +/-   ##
==========================================
+ Coverage   93.40%   93.44%   +0.03%     
==========================================
  Files          27       27              
  Lines        3414     3416       +2     
==========================================
+ Hits         3189     3192       +3     
+ Misses        225      224       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

arthur-brigatto and others added 3 commits May 22, 2024 13:42
Add test function for sample_backward_noise_terms_with_state
@arthur-brigatto
Copy link
Contributor Author

@odow I have added a test function. The idea is: Firstly a sequence of 0.1 or 0.9 values for $\epsilon$ is sampled in the forward step. Then, the backward sampler sets $\epsilon = 0.9$ if 0.1 was sampled in the forward step or it sets $\epsilon = 0.1$ if 0.9 was sampled in the forward step.

Let me know if that works!

src/plugins/headers.jl Outdated Show resolved Hide resolved
@odow odow changed the title Add the possibility of using the outgoing state in the backward sampling step [WIP] Add sample_backward_noise_terms_with_state Jun 4, 2024
src/plugins/headers.jl Outdated Show resolved Hide resolved
@odow odow merged commit fc2d2a8 into odow:master Jun 4, 2024
7 checks passed
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.

Error when using nonlinear function of state.in
3 participants