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

Documentation feedback #432

Closed
16 of 17 tasks
odow opened this issue Jun 27, 2021 · 1 comment
Closed
16 of 17 tasks

Documentation feedback #432

odow opened this issue Jun 27, 2021 · 1 comment

Comments

@odow
Copy link
Owner

odow commented Jun 27, 2021

An unnamed person emailed me this feedback for the documentation. I think it's fair. Making it public to motivate me to fix it.

  • Basic I: s/probalistic/probabilistic

  • Why not include a policy graph in the hydro-thermal problem ?

  • Basic II: JuMP.fix link is not working.

  • There was always something that bothered me about Figure 17, and now I know what it is: you have markov states for prices AND THERE IS STILL UNCERTAINTY IN THE MODEL THAT IS NOT PRICE. That makes things very hard, if not impossible, for a reader to grasp. In the first Figure (the one with the 1,2,3 ) everything is abstract and it is not clear what a state is. Then you put an example where price is a state, which IS NOT NATURAL because when people think of price they think of the jiggling arrow of randomness. I may be the only person in the work that knows that the jiggling arrows in Figure 17 represent evapotranspiration and rainfall (am I correct?). That is why the el niño/la niña example is great. The one from this paper
    https://link.springer.com/article/10.1007/s10479-018-2991-z
    is also good, because people understand states as, well, states of the world.

Fixed in #439

  • Along the same lines, I went to the original POWDer paper and in what you call Figure 3 you say that it is a SCENARIO TREE!!! Now I am super confused, and a reader who goes to the original references will probably give up. Figure 3 in the paper and Figure 17 in SDDP.jl are completely different, and totally similar at the same time!!!

Different figures from different papers.

  • minor things: in the hydro problem, using firefox I see $ all the time in the example.

I think this is actually a bug in Documenter Confirmed upstream: JuliaDocs/Documenter.jl#890

  • It is hard to see what is computational input and what is computational output in the tutorial.

This is the default Documenter theme. There may be ways to change it.

  • outgoing_volume = [stage[:volume].out for stage in simulations[1]] Oh my, for such a simple thing as a display of a variable, this is super unfriendly! I would have never in my life figured this out...
    I can't even parse it...

Notation fixed in #439

  • Typo: "with resspect to each state variable."

  • Extracting the water values there is something I don't get here. The gradient is the derivative of Q_t wrt to v_t, where Q_t(v_t), Right? What is "cost"? a) the value of Q_t(v_{t-1})? b) the estimated value of Q_t(v_{t-1})? c) the value of Q_{t+1}(v_t)? I am guessing a)

  • There is also a method of SDDP.evaluate that takes a dictionary as an argument for cases where the keyword argument doesn't work. That makes the reader anxious. Why wouldn't the keyword argument work???

Removed reference to the mysterious keyword arguments. That came up previously.

  • Which brings me to the last point, which probably you would have guessed.
    After all the introduction on PG, discussions on different ways of representing uncertainty, omegas everywhere (V_i, S_i, P_i, U_i. T_i),
    I was surprised the example did not contain uncertainty. OK, I understand the rationale, you isolated the other elements and postponed the inclusion of uncertainty for later.
    I am not the owner of the truth, I do not think it was the best choice. The "Basic I" section is a considerable investment in terms of time, and uncertainty is clearly a key element
    in all that conversation.

Fixed in #439

  • I don't think you mentioned what the column "simulation" means in the output. This is a forward pass, right? Why would the user want to see that? This number could be anything, it is not decreasing, correct? The bound is always increasing for a min problem, I get that.

Fixed in #439

  • This is more like a SDDP-comment:
julia> println("Confidence interval: ", μ, " ± ", ci)
Confidence interval: 8080.0 ± 415.63

julia> println("Lower bound: ", round(SDDP.calculate_bound(model), digits = 2))
Lower bound: 8333.33

Ugly to see the central point of the confidence interval way below the lower bound. Maybe warn the reader that this could happen?

It's a pretty wide confidence interval. I don't see the issue.

  • In the very last part (custom_recorders) it took me sometime to grasp that you have a sample path,
    and you are taking the duals of the constraints defined at each node. t wouldn't hurt to remind the reader that you
    using the simulate to generate a single path, and the balance constraint will be defined for each inflow sampled at each node.

Clarified in #439

  • Final comment, connecting to what I said on 2/n: this section is super short, now I am 100% convinced the gain in NOT considering uncertainty in the first example is very small compared to the benefits of seeing uncertainty appear right away.

Revised in #439

  • "You don't have just return values from the simulation, you can compute things too." I am missing a "to" between "have" and "just" in this sentence, but I am not a native speaker, so...
@odow
Copy link
Owner Author

odow commented Jul 6, 2021

I made a fix for the documenter $ issue: JuliaDocs/Documenter.jl#1625

@odow odow closed this as completed Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant