-
Notifications
You must be signed in to change notification settings - Fork 5
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
Julia1.0 #4
Julia1.0 #4
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4 +/- ##
==========================================
- Coverage 49.53% 43.41% -6.13%
==========================================
Files 6 7 +1
Lines 216 205 -11
==========================================
- Hits 107 89 -18
- Misses 109 116 +7
Continue to review full report at Codecov.
|
examples/HydroValleys/case3.jl
Outdated
using Base.Test | ||
@test isapprox(results["simulations"][1]["objective"], 12400.00, atol=1e-2) | ||
using Test | ||
@test isapprox(results[:simulations][1][1][:objective], 10496.09, atol=1) |
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.
Hmm. To get the old behaviour you need something like
sum(s[:stage_objective] for s in results[:simulations][1])
We could make this nicer.
objective_bound = 0.0 | ||
optimizer = with_optimizer(params["optimizer"]), | ||
lower_bound = 0.0, | ||
direct_mode=false |
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.
Hmm. I should probably document this better.
using Base.Test | ||
@test isapprox(results["simulations"][1]["objective"], 12400.00, atol=1e-2) | ||
using Test | ||
@test isapprox(sum(s[:stage_objective] for s in results[:simulations][1]), 9400.0, atol=1) |
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 did the value (12400) change? Is it because you run more iterations? (I can't guarantee that SDDP.jl is bug-free, so changes like this are worth investigating.)
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.
It doesn't seem to be because I run more iterations. I augmented it because with 60 iterations, in the new framework, the sddp was not returning a stable solution.
My best guess is that the scenarios are not being sampled in the same way. I tried to seed it, but no luck. Do you believe this could be the reason?
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.
Oh, it's probably because I turn cut selection on by default now. You can set the cut_deletion_minimum
argument in train to a large number like 1_000_000
if you want to turn it off.
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.
Just added the cut_deletion_minimum
argument. However, I was not doing this on the old release. Couldn't this be because the random generator has actually changed and my seed doesn't work the same way?
I was advise to start testing the lower-bound instead of the simulation cost.
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.
Potentially. I usually check the lower bound:
https://github.com/odow/SDDP.jl/blob/9fea8fcd3b6ab028b94835ccb6617f4d4d9a4b5c/examples/FAST_hydro_thermal.jl#L37
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.
Will do this from now on!
using Base.Test | ||
@test isapprox(results["simulations"][1]["objective"], 59800.0, atol=1e-2) | ||
using Test | ||
@test isapprox(results[:simulations][1][1][:objective], 58528.47, atol=1) |
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.
Ditto on the value.
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.
Changed to sum(s[:stage_objective] for s in results[:simulations][1]), but still different.
Use import or using? |
Probably using. It's more user friendly. Within a package, I'm trying to mainly use import. The reexport is a thorn though. |
Code to julia 1.0.