ccoffrin commented 4 years ago

closes #28, closes #30, closes #31, closes #33

@noahrhodes, some requests for you,

please review run_iterative_restoration in detail.
From what I can tell the functions core/data.jl/count_active_items and core/data.jl/get_active_items are only used in the tests. Do they have broader utility? If not, these should be moved out of the core package and into test/common.jl where testing helper functions live.
As a side note, the return type of count_active_items is not stable, it can be an Int or a Dict this is generally discouraged. It seems like only the Int version is used in the tests, so maybe we can reduce the scope to only use the Int path.
There is an open question on what the status values in network 0 solution should be. Specifically, should the components that are active at the start of the restoration have explicit status values set to 1? My feeling is not, becouse the convention is that values that are not decision variables in the model do not appear in the solution; but this may be a inconsistent with the convention of the ROP formulation that occurs in the 1+ network solutions. This goes back to the border point if the initial-state should be modeled explicitly or not. At least in the near term, the API of run_iterative_restoration should match that of run_rop models.

codecov[bot] commented 4 years ago

Codecov Report

Merging #37 into master will decrease coverage by 2.42%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
- Coverage   92.81%   90.38%   -2.43%     
==========================================
  Files          22       22              
  Lines        1447     1404      -43     
==========================================
- Hits         1343     1269      -74     
- Misses        104      135      +31

Impacted Files	Coverage Δ
src/util/iterative_restoration.jl	`84.79% <0.00%> (-12.35%)`	:arrow_down:
src/core/data.jl	`80.93% <0.00%> (-1.89%)`	:arrow_down:

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 216d623...30ab870. Read the comment docs.

ccoffrin commented 4 years ago

Note, tests are passing on os x. I have an inquiry out to @blegat, to see if he has any insights on the linux issue.

noahrhodes commented 4 years ago

As for using the "initial state network 0", my view on it is that we have a base damaged network. Then:

A repair is selected in network 0.
It takes time to repair (storage energy is going down)
The device is repaired and active in network 1

The initial condition and the time that the repair takes matters in the context of energy storage. My view is that the device becomes active in network N but repairs happened during the previous network.

ccoffrin commented 4 years ago

@noahrhodes, I moved the active_items as discussed. My current plan is to tag v0.2.1, then wait a day or two to see if we can get some insights on the cause of the Linux failure. If not, we will omit that test, merge this and tag v0.3.

We should have a call to discuss the explicit modeling of initial state and we can plan any updates along those lines after this release.

ccoffrin commented 4 years ago

Let me know if you have any reason to delay this plan.

ccoffrin commented 4 years ago

@noahrhodes, to close the loop up on this point. The core issue with the Linux seems to be a JuMP bug (details here, https://github.com/JuliaOpt/JuMP.jl/issues/2187). So I have commented out the test until the issue is resolved in JuMP. If you run into the issue in practice, you can work around it by deactivating buses where the error occurs.

lanl-ansi / PowerModelsRestoration.jl

Updates for PowerModels v0.15 #37

Codecov Report