-
Notifications
You must be signed in to change notification settings - Fork 61
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
Conversation
…de) to sample_backward_noise_terms(backward_sampling_scheme, child_node, outgoing_state)
Ab/backward sampling with state
A simple test would be great to see if it works! |
You can add a new test as a function just after this one: SDDP.jl/test/plugins/backward_sampling_schemes.jl Lines 66 to 90 in 8ed2e30
|
src/plugins/headers.jl
Outdated
@@ -112,6 +112,7 @@ abstract type AbstractBackwardSamplingScheme end | |||
sample_backward_noise_terms( | |||
backward_sampling_scheme::AbstractBackwardSamplingScheme, | |||
node::Node{T}, | |||
state::Dict{Symbol,Float64}, |
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.
Okay, so:
I want two methods:
- the existing
sample_backward_noise_terms
- 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.
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.
Alright, I had the wrong idea. I will correct it.
Co-authored-by: Oscar Dowson <[email protected]>
Co-authored-by: Oscar Dowson <[email protected]>
Co-authored-by: Oscar Dowson <[email protected]>
Co-authored-by: Oscar Dowson <[email protected]>
Working on it! |
Co-authored-by: Oscar Dowson <[email protected]>
Co-authored-by: Oscar Dowson <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Add test function for sample_backward_noise_terms_with_state
@odow I have added a test function. The idea is: Firstly a sequence of 0.1 or 0.9 values for Let me know if that works! |
Closes #735.