Skip to content

default sampler#16

Open
leclere wants to merge 4 commits intomasterfrom
vl/sample
Open

default sampler#16
leclere wants to merge 4 commits intomasterfrom
vl/sample

Conversation

@leclere
Copy link
Copy Markdown
Contributor

@leclere leclere commented Jul 25, 2018

No description provided.

@leclere leclere requested a review from blegat July 25, 2018 14:04
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 25, 2018

Codecov Report

Merging #16 into master will decrease coverage by 3.72%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/attributes.jl 0% <0%> (ø) ⬆️
src/algorithm.jl 68.29% <0%> (-24.57%) ⬇️
src/info.jl 70.27% <0%> (+16.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07e612b...6ae5a6d. Read the comment docs.

Comment thread src/algorithm.jl Outdated
"""
function sample_scenarios end

function sample_scenarios(sp::AbstractStochasticProgram, n::Int, depth_max = 1000,to::TimerOutput=TimerOutput(), verbose::Int=0)
Copy link
Copy Markdown
Member

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.

Comment thread src/algorithm.jl Outdated

function sample_scenarios(sp::AbstractStochasticProgram, n::Int, depth_max = 1000,to::TimerOutput=TimerOutput(), verbose::Int=0)
scenarios = []
for i = 1:n
Copy link
Copy Markdown
Member

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

Comment thread src/algorithm.jl Outdated

function sample_scenario(sp::AbstractStochasticProgram, depth_max = 1000,to::TimerOutput=TimerOutput(), verbose::Int=0)
node = get(sp,MasterNode())
s = []
Copy link
Copy Markdown
Member

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?

Comment thread src/attributes.jl Outdated
```
"""
struct RandomTransition <: AbstractNodeAttribute end
function get(sp::AbstractStochasticProgram, tr::RandomTransition,node::Int)
Copy link
Copy Markdown
Member

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

Comment thread src/attributes.jl Outdated
RandomTransition <: AbstractNodeAttribute

return a randomly selected transition from node `node`
### Examples
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is it indented?

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where?

Comment thread src/attributes.jl Outdated
struct RandomTransition <: AbstractNodeAttribute end
function get(sp::AbstractStochasticProgram, tr::RandomTransition,node::Int)
r = rand()
cdf = 0
Copy link
Copy Markdown
Member

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

Comment thread src/attributes.jl Outdated

struct IsLeaf <: AbstractNodeAttribute end
function get(sp::AbstractStochasticProgram, node::Int)
return isempty(get(sp,OutTransitions(),node))
Copy link
Copy Markdown
Member

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

Comment thread src/attributes.jl Outdated
return isempty(get(sp,OutTransitions(),node))
end
function is_empty(sp::AbstractStochasticProgram, node::Int)
return get(sp,IsLeaf(),node)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Idem

Comment thread src/algorithm.jl
s = Vector{:<AbstractTransition}
it = 0
while !is_leaf(sp,node) && it < depth_max
tr = get(sp,RandomTransition,node)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

RandomTransition -> RandomTransition ()

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.

3 participants