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

trans rewrite #187

Merged
merged 10 commits into from
Nov 19, 2019
Merged

trans rewrite #187

merged 10 commits into from
Nov 19, 2019

Conversation

sanderclaeys
Copy link
Contributor

I rewrote the transformer methods as preparation for the convex relaxations of transformers and OLTC. Like this the ACP formulation is more readable, and it will fit the relaxations better as well.

The dispatch to OLTC or fixed transformer might need some discussion.
For me, the default problem should include OLTCs. If the user does not want OLTCs, she should fix the taps in the data model. The formulations should then resort automatically to the simplified equations instead for fixed taps. Last time, I remember there was some preference for ignoring the fixed setting and instead considering it as fixed in most problem definitions. I considered both by implementing the following logic.

  • constraint_mc_oltc in the constraint template calls constraint_mc_trans_yy or constraint_mc_trans_dy depending on the winding types, and passes the fixed parameter of the data model. These methods are OLTC ready, and can simplify the equations when all/some of the taps are fixed.
  • constraint_mc_trans is identical to constraint_mc_oltc, but ignores the data model and passes a vector of ones, indicating all taps are fixed to constraint_mc_trans_yy and constraint_mc_trans_dy.

This achieves a good balance between code duplication, readability and exploiting fixed variables to reduce the complexity where possible.

@codecov
Copy link

codecov bot commented Nov 17, 2019

Codecov Report

Merging #187 into master will decrease coverage by 1.52%.
The diff coverage is 90.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
- Coverage   89.93%   88.41%   -1.53%     
==========================================
  Files          39       39              
  Lines        4013     4056      +43     
==========================================
- Hits         3609     3586      -23     
- Misses        404      470      +66
Impacted Files Coverage Δ
src/form/apo.jl 89.28% <ø> (-0.37%) ⬇️
src/form/bf.jl 97.91% <ø> (-0.05%) ⬇️
src/core/variable.jl 100% <100%> (ø) ⬆️
src/form/acp.jl 93.24% <100%> (-6.76%) ⬇️
src/prob/opf_oltc.jl 86.36% <100%> (ø) ⬆️
src/core/constraint_template.jl 96.59% <75%> (-1.69%) ⬇️
src/core/data.jl 90.76% <81.25%> (-3.11%) ⬇️
src/core/ref.jl 42.66% <0%> (-47.34%) ⬇️
src/form/shared.jl 91.42% <0%> (-4.29%) ⬇️

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 dc1b2e3...990011f. Read the comment docs.

@pseudocubic pseudocubic self-assigned this Nov 18, 2019
@pseudocubic pseudocubic self-requested a review November 18, 2019 15:03
@pseudocubic pseudocubic added the Type: Enhancement New feature or request label Nov 18, 2019
Copy link
Collaborator

@pseudocubic pseudocubic left a comment

Choose a reason for hiding this comment

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

Overall this is a really good cleanup. I have some relatively minor items that I think should be addressed, and a line should be added to the changelog, but otherwise I think this is in good shape

src/core/constraint_template.jl Outdated Show resolved Hide resolved
src/core/constraint_template.jl Outdated Show resolved Hide resolved
src/core/data.jl Outdated Show resolved Hide resolved
src/core/data.jl Outdated Show resolved Hide resolved
src/core/constraint_template.jl Outdated Show resolved Hide resolved
src/core/variable.jl Outdated Show resolved Hide resolved
src/form/acp.jl Outdated Show resolved Hide resolved
src/form/acp.jl Outdated Show resolved Hide resolved
src/form/acp.jl Outdated Show resolved Hide resolved
@pseudocubic
Copy link
Collaborator

@sanderclaeys to address the point you bring up for discussion about whether oltc should be specified when instantiating a problem definition or by the data model, I personally am inclined towards your preference of specifying this in the data. As long as this feature is well documented I don't think there should be any problem.

That being said, I think that this issue should be left to a separate PR after this and #150 are resolved, when we revisit the problem definitions in the very near future

@sanderclaeys
Copy link
Contributor Author

Sounds good, I will create an issue for this as a reminder.

Also, the transformer code now implicitly assumes that each conductor is also a phase; this code will break for 4-wire systems, where the neutral is part of _PMs.conductor_ids(pm). This should be easy enough to update once we decide on a pattern for neutral vs. phase conductors.

@pseudocubic pseudocubic added the Category: Formulations Network Formulation label Nov 18, 2019
@pseudocubic pseudocubic merged commit 90f0b40 into lanl-ansi:master Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Formulations Network Formulation Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants