-
Notifications
You must be signed in to change notification settings - Fork 1
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
default sampler #16
base: master
Are you sure you want to change the base?
default sampler #16
Conversation
Codecov Report
@@ Coverage Diff @@
## master #16 +/- ##
==========================================
- Coverage 55.42% 51.69% -3.73%
==========================================
Files 4 4
Lines 83 118 +35
==========================================
+ Hits 46 61 +15
- Misses 37 57 +20
Continue to review full report at Codecov.
|
src/algorithm.jl
Outdated
@@ -112,11 +112,33 @@ algorithm `algo` with verbose level `verbose`. | |||
""" | |||
function sample_scenarios end | |||
|
|||
function sample_scenarios(sp::AbstractStochasticProgram, n::Int, depth_max = 1000,to::TimerOutput=TimerOutput(), verbose::Int=0) |
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.
80C.
For consistency, we should keep the PEP8 all along :
- add space after comma:
, to::...
- no space before and after equal symbol "=" in function arguments.
src/algorithm.jl
Outdated
@@ -112,11 +112,33 @@ algorithm `algo` with verbose level `verbose`. | |||
""" | |||
function sample_scenarios end | |||
|
|||
function sample_scenarios(sp::AbstractStochasticProgram, n::Int, depth_max = 1000,to::TimerOutput=TimerOutput(), verbose::Int=0) | |||
scenarios = [] | |||
for i = 1:n |
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 would tend to follow JuMP guideline for for
loop:
https://github.com/JuliaOpt/JuMP.jl/blob/master/docs/src/style.md#for-loops
src/algorithm.jl
Outdated
|
||
function sample_scenario(sp::AbstractStochasticProgram, depth_max = 1000,to::TimerOutput=TimerOutput(), verbose::Int=0) | ||
node = get(sp,MasterNode()) | ||
s = [] |
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 would say that s
is not that explicit :p
Is it possible to type the array?
src/attributes.jl
Outdated
``` | ||
""" | ||
struct RandomTransition <: AbstractNodeAttribute end | ||
function get(sp::AbstractStochasticProgram, tr::RandomTransition,node::Int) |
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 would add a space between ,
and node::Int
src/attributes.jl
Outdated
RandomTransition <: AbstractNodeAttribute | ||
|
||
return a randomly selected transition from node `node` | ||
### Examples |
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.
Why is it indented?
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.
Following other example in SOI
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.
Where?
src/attributes.jl
Outdated
struct RandomTransition <: AbstractNodeAttribute end | ||
function get(sp::AbstractStochasticProgram, tr::RandomTransition,node::Int) | ||
r = rand() | ||
cdf = 0 |
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 would use : cdf = 0.
for type stability
src/attributes.jl
Outdated
|
||
struct IsLeaf <: AbstractNodeAttribute end | ||
function get(sp::AbstractStochasticProgram, node::Int) | ||
return isempty(get(sp,OutTransitions(),node)) |
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 would add spaces after comma
src/attributes.jl
Outdated
return isempty(get(sp,OutTransitions(),node)) | ||
end | ||
function is_empty(sp::AbstractStochasticProgram, node::Int) | ||
return get(sp,IsLeaf(),node) |
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.
Idem
s = Vector{:<AbstractTransition} | ||
it = 0 | ||
while !is_leaf(sp,node) && it < depth_max | ||
tr = get(sp,RandomTransition,node) |
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.
RandomTransition -> RandomTransition ()
No description provided.