UPD: Updates for new multi-infrastructure conventions #222

tasseff · 2020-10-26T17:35:36Z

This pull request coincides with the multi-infrastructure branch of InfrastructureModels.jl.

tasseff · 2020-12-04T00:10:49Z

The automated tests will not pass until the new version of InfrastructureModels is released, but this is probably sufficient for a review.

…writing.

…nfrastructureModels changes.

codecov · 2021-01-18T22:37:51Z

Codecov Report

Merging #222 (d97bfd8) into master (de11ed5) will increase coverage by 0.81%.
The diff coverage is 83.47%.

@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
+ Coverage   77.61%   78.43%   +0.81%     
==========================================
  Files          35       35              
  Lines        4571     4624      +53     
==========================================
+ Hits         3548     3627      +79     
+ Misses       1023      997      -26

Impacted Files	Coverage Δ
src/GasModels.jl	`25.00% <ø> (ø)`
src/core/export.jl	`100.00% <ø> (ø)`
src/core/unit_converters.jl	`0.00% <ø> (ø)`
src/form/dwp.jl	`76.74% <ø> (+1.74%)`	⬆️
src/form/lrdwp.jl	`43.47% <ø> (+5.74%)`	⬆️
src/io/diagnostics.jl	`0.00% <0.00%> (ø)`
src/prob/gf.jl	`86.95% <ø> (ø)`
src/prob/ls.jl	`74.41% <ø> (ø)`
src/prob/ne.jl	`70.00% <ø> (ø)`
src/prob/nels.jl	`68.33% <ø> (ø)`
... and 28 more

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 de11ed5...e38c121. Read the comment docs.

tasseff · 2021-02-19T20:32:28Z

This will close most of #226, aside from the run_ to solve_ renaming, which @ccoffrin has recommended we delay. I think it's safe for me to pass this off to the most frequent contributors for review, editing, and merging.

ccoffrin

Overall works for me. Put in a few minor suggestions.

ccoffrin · 2021-02-20T00:14:44Z

examples/run_examples.jl

+couenne_solver = JuMP.with_optimizer(AmplNLWriter.Optimizer("couenne.exe"))
+bonmin_solver = JuMP.with_optimizer(AmplNLWriter.Optimizer("bonmin.exe"))


Should avoid the use of with_optimizer, I think new syntax in this case is couenne_solver = () -> AmplNLWriter.Optimizer("couenne.exe")

Noted. It looks like these solvers weren't being used, so I also commented them out.

ccoffrin · 2021-02-20T00:18:27Z

src/core/data.jl

-                @assert data["is_per_unit"] == 1
-                comp["is_per_unit"] = data["is_per_unit"]
+
+            if ~haskey(comp, "is_per_unit") && haskey(gm_data, "is_per_unit")


Not specifically related to this PR but ! is preferable to ~.

ccoffrin · 2021-02-20T00:21:33Z

src/core/export.jl

+import JuMP: optimizer_with_attributes
+export optimizer_with_attributes



FYI, here I also added nw_id_default to exports. But I am still on the fence if that is the best choice.

ccoffrin · 2021-02-20T00:26:15Z

src/core/transient_variable.jl

-        _IM.sol_component_value(gm, nw, :junction, :density, ids(gm, nw, :junction), rho)
+        _IM.sol_component_value(gm, gm_it_sym, nw, :junction, :density, ids(gm, nw, :junction), rho)
    report &&
-        _IM.sol_component_value(gm, nw, :junction, :pressure, ids(gm, nw, :junction), rho)
+        _IM.sol_component_value(gm, gm_it_sym, nw, :junction, :pressure, ids(gm, nw, :junction), rho)


This is a stylistic point, but in PowerModels we implemented a package specific version of sol_component_value that is specialized on AbstractPowerModel, so that pm_it_sym does not need to be used explicitly within the package.

https://github.com/lanl-ansi/PowerModels.jl/blob/rc-v0.18/src/core/solution.jl#L28

ccoffrin · 2021-02-20T00:28:28Z

test/data.jl

+            var(gm, InfrastructureModels.nw_id_default, :pipe)[32] === nothing
+            var(gm, InfrastructureModels.nw_id_default, :pipe)[34] === nothing


Can be simplified if you choose to export nw_id_default.

ccoffrin · 2021-02-20T00:29:57Z

test/runtests.jl

+Logging.disable_logging(Logging.Info)
+Logging.disable_logging(Logging.Warn)


Just curious, given that we use Memento for logging, wondering why this is here as well.

Juniper appears to use Logging instead of Memento. This is mostly for disabling Juniper info and warning output.

kaarthiksundar

Looks great. Thanks Byron.

tasseff added 7 commits October 22, 2020 12:56

Committing working portions of multi-infrastructure implementation.

93b245e

Continuing to work through refactor.

5865599

Trying to work through transient data wrangling.

5de5465

Uncomment tests.

658dcb2

Need to fix the "debug" tests.

2861300

Updating debug test.

d31aa55

Update CHANGELOG.

40b4376

tasseff requested a review from rb004f October 26, 2020 17:36

Fix solution preprocessor.

c618590

tasseff requested a review from ccoffrin October 26, 2020 19:00

tasseff added 8 commits November 4, 2020 10:44

Migrating to newer InfrastructureModels functions.

95dc54d

Ensure all tests pass with new InfrastructureModels functions.

50e979a

Define constants for infrastructure type.

18c046e

Use new global infrastructure type symbols.

3e65728

Update multi-infrastructure functions.

03985d6

Multi-infrastructure conventions.

601b8ef

Return objective function.

f10f82e

Add "junction_id" as a possible component junction key.

fc2ddb6

tasseff marked this pull request as ready for review December 4, 2020 00:10

tasseff and others added 6 commits December 24, 2020 10:36

Correct comparisons, update dependencies, change precision in MATGAS …

e72ce8f

…writing.

Modify infrastructure symbol and naming variables.

abee5e9

Move helper functions to base.

a01ae96

Use new apply! and apply_gm! functions.

7e04f5e

Migrate to new multi-infrastructure functions.

65acfaa

Merge branch 'master' into multi-infrastructure. Incorporate recent I…

1ac0d1b

…nfrastructureModels changes.

tasseff and others added 3 commits January 19, 2021 09:12

Use new instantiate_model arguments.

091eaf3

Fix for MATGAS export.

eac2eb4

Addresses much of #226.

87eae55

tasseff requested review from kaarthiksundar and pseudocubic February 19, 2021 20:32

ccoffrin approved these changes Feb 20, 2021

View reviewed changes

tasseff added a commit that referenced this pull request Feb 24, 2021

Addressing comments from @cjc on #222.

9d87ecc

Addressing comments from @ccoffrin on #222.

e38c121

tasseff force-pushed the multi-infrastructure branch from 9d87ecc to e38c121 Compare February 24, 2021 19:13

kaarthiksundar approved these changes Feb 27, 2021

View reviewed changes

tasseff merged commit 5b5f62c into master Mar 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UPD: Updates for new multi-infrastructure conventions #222

UPD: Updates for new multi-infrastructure conventions #222

tasseff commented Oct 26, 2020

tasseff commented Dec 4, 2020 •

edited

Loading

codecov bot commented Jan 18, 2021 •

edited

Loading

tasseff commented Feb 19, 2021

ccoffrin left a comment

ccoffrin Feb 20, 2021

tasseff Feb 24, 2021

ccoffrin Feb 20, 2021

ccoffrin Feb 20, 2021

ccoffrin Feb 20, 2021

ccoffrin Feb 20, 2021

ccoffrin Feb 20, 2021

tasseff Feb 24, 2021

kaarthiksundar left a comment

		couenne_solver = JuMP.with_optimizer(AmplNLWriter.Optimizer("couenne.exe"))
		bonmin_solver = JuMP.with_optimizer(AmplNLWriter.Optimizer("bonmin.exe"))

		import JuMP: optimizer_with_attributes
		export optimizer_with_attributes

		var(gm, InfrastructureModels.nw_id_default, :pipe)[32] === nothing
		var(gm, InfrastructureModels.nw_id_default, :pipe)[34] === nothing

		Logging.disable_logging(Logging.Info)
		Logging.disable_logging(Logging.Warn)

UPD: Updates for new multi-infrastructure conventions #222

UPD: Updates for new multi-infrastructure conventions #222

Conversation

tasseff commented Oct 26, 2020

tasseff commented Dec 4, 2020 • edited Loading

codecov bot commented Jan 18, 2021 • edited Loading

Codecov Report

tasseff commented Feb 19, 2021

ccoffrin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaarthiksundar left a comment

Choose a reason for hiding this comment

tasseff commented Dec 4, 2020 •

edited

Loading

codecov bot commented Jan 18, 2021 •

edited

Loading