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

add Yft, Ytf to power_network_matrix Ybus #95

Merged
merged 16 commits into from
Feb 5, 2025
Merged

add Yft, Ytf to power_network_matrix Ybus #95

merged 16 commits into from
Feb 5, 2025

Conversation

rbolgaryn
Copy link
Contributor

Thanks for opening a PR to PowerNetworkMatrices.jl, please take note of the following when making a PR:

Check the contributor guidelines

@rbolgaryn rbolgaryn self-assigned this Jan 14, 2025
@rbolgaryn rbolgaryn added the enhancement New feature or request label Jan 14, 2025
@rbolgaryn
Copy link
Contributor Author

Add branch admittance matrices (Yft, Ytf) to the Ybus structure so that we can efficiently calculate branch flows in PowerFlows in AC power flow calculation.

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Performance Results

Version Precompile Time
Main 2.430159649
This Branch 2.449701756
Version Execute Time
Main-Build PTDF First 5.821146634
Main-Build PTDF Second 0.093171718
Main-Build Ybus First 0.562123225
Main-Build Ybus Second 0.007672568
Main-Build LODF First 1.414734528
Main-Build LODF Second 0.217402243
This Branch-Build PTDF First 6.058768057
This Branch-Build PTDF Second 0.091896339
This Branch-Build Ybus First 0.595320011
This Branch-Build Ybus Second 0.00616453
This Branch-Build LODF First 1.291518817
This Branch-Build LODF Second 0.35974152

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 84.74576% with 9 lines in your changes missing coverage. Please review.

Project coverage is 77.83%. Comparing base (7069dca) to head (0387d4b).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
src/ybus_calculations.jl 84.74% 9 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #95      +/-   ##
==========================================
+ Coverage   77.61%   77.83%   +0.22%     
==========================================
  Files          14       14              
  Lines        1398     1430      +32     
==========================================
+ Hits         1085     1113      +28     
- Misses        313      317       +4     
Flag Coverage Δ
unittests 77.83% <84.74%> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/ybus_calculations.jl 47.78% <84.74%> (+6.55%) ⬆️

Roman Bolgaryn and others added 2 commits January 14, 2025 16:28
formatting

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@GabrielKS GabrielKS left a comment

Choose a reason for hiding this comment

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

A few minor things. I might not yet fully understand the use case. Also, this is missing tests.

src/ybus_calculations.jl Outdated Show resolved Hide resolved
src/ybus_calculations.jl Outdated Show resolved Hide resolved
src/ybus_calculations.jl Outdated Show resolved Hide resolved
Copy link
Member

@jd-lara jd-lara left a comment

Choose a reason for hiding this comment

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

These additional entries are in fact different Matrices. They should be separated into their own object and potentially store just one matrix but implement methods that allow the calculations to be done efficiently.

@rbolgaryn
Copy link
Contributor Author

This PR should be good now

Copy link
Member

@jd-lara jd-lara left a comment

Choose a reason for hiding this comment

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

Add a kwarg to the Ybus constructor to store the additional matrices and create nullable fields for the additional matrices and the branches index.

@rbolgaryn
Copy link
Contributor Author

rbolgaryn commented Jan 31, 2025

todo: 1) make also the ft, tb, sb, nullable (don"t populate those either), 2) update docstrings for Ybus struct

Copy link
Contributor

@GabrielKS GabrielKS left a comment

Choose a reason for hiding this comment

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

Deferring to @jd-lara to evaluate the design on the whole

src/ybus_calculations.jl Outdated Show resolved Hide resolved
typo fix

Co-authored-by: Gabriel Konar-Steenberg <[email protected]>
src/ybus_calculations.jl Outdated Show resolved Hide resolved
Copy link
Member

@jd-lara jd-lara left a comment

Choose a reason for hiding this comment

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

There is one small change to consider about the branch indexing

@rbolgaryn rbolgaryn requested a review from jd-lara February 5, 2025 00:50
@jd-lara jd-lara merged commit ec079ad into main Feb 5, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants