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

default sampler #16

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

default sampler #16

wants to merge 4 commits into from

Conversation

leclere
Copy link
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

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.

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)
Copy link
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.

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
Copy link
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

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
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?

```
"""
struct RandomTransition <: AbstractNodeAttribute end
function get(sp::AbstractStochasticProgram, tr::RandomTransition,node::Int)
Copy link
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

RandomTransition <: AbstractNodeAttribute

return a randomly selected transition from node `node`
### Examples
Copy link
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
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
Member

Choose a reason for hiding this comment

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

Where?

struct RandomTransition <: AbstractNodeAttribute end
function get(sp::AbstractStochasticProgram, tr::RandomTransition,node::Int)
r = rand()
cdf = 0
Copy link
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


struct IsLeaf <: AbstractNodeAttribute end
function get(sp::AbstractStochasticProgram, node::Int)
return isempty(get(sp,OutTransitions(),node))
Copy link
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

return isempty(get(sp,OutTransitions(),node))
end
function is_empty(sp::AbstractStochasticProgram, node::Int)
return get(sp,IsLeaf(),node)
Copy link
Member

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)
Copy link
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