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

Extend Branch definition to include transformers #6

Merged
merged 6 commits into from
May 25, 2022
Merged

Conversation

BSnelling
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #6 (135b2ca) into main (d5ae93a) will increase coverage by 4.32%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main       #6      +/-   ##
==========================================
+ Coverage   75.26%   79.59%   +4.32%     
==========================================
  Files           2        2              
  Lines          93       98       +5     
==========================================
+ Hits           70       78       +8     
+ Misses         23       20       -3     
Impacted Files Coverage Δ
src/accessors.jl 98.38% <100.00%> (ø)
src/system.jl 47.22% <100.00%> (+18.18%) ⬆️

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 d5ae93a...135b2ca. Read the comment docs.

src/system.jl Outdated
@@ -99,6 +99,60 @@ struct Branch
break_points::Tuple{Float64, Float64}
"Price penalties for each of the break points of the branch (\$)"
penalties::Tuple{Float64, Float64}
"Resistance of the transformer (Ohm)"
resistance::Float64
"Reactance of the transformer or branch (Ohm)"
Copy link
Member Author

Choose a reason for hiding this comment

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

What do we want the units of Resistance and Reactance to be?

Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in the discussion we had in the FNDP PR, I think having all branch parameters in pu is the best course unfortunately. It just makes everything much simpler, otherwise we'd need to rely on conversions whenever doing operations with parameters from different branches.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should rate_a and rate_b also be in pu? Or are they ok in MVA.

Copy link
Member

Choose a reason for hiding this comment

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

I think for the sake of simplicity we can postpone the pu discussion to later when we already have things figured out. Let's just keep everything as pu for now. For things that are in MW/MVA, we can either rely on base_power being stored somewhere or document that we assume base = 100 MW.

src/system.jl Outdated Show resolved Hide resolved
src/system.jl Outdated Show resolved Hide resolved
test/system.jl Outdated Show resolved Hide resolved
@BSnelling BSnelling marked this pull request as draft May 24, 2022 15:57
@BSnelling BSnelling marked this pull request as ready for review May 24, 2022 23:07
src/system.jl Outdated
Comment on lines 102 to 105
"Resistance of the transformer (pu)"
resistance::Float64
"Reactance of the transformer or branch (pu)"
reactance::Float64
Copy link
Member

Choose a reason for hiding this comment

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

Given that transformer is a subset of branch, I'd just say "branch" here – "transformer or branch" is confusing

I'd also say branch for the resistance as it makes sense for lines to have resistance even though we don't use it in the formulation or to compute shift factors

@nickrobinson251
Copy link
Contributor

LGTM

@BSnelling BSnelling merged commit f8f22dc into main May 25, 2022
@BSnelling BSnelling deleted the bes/transformers branch May 25, 2022 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants