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.

Sign up for GitHub

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

Jump to bottom

loss factors as matrix in PowerFlowData #98

Merged
merged 8 commits into from
Feb 18, 2025
Merged

loss factors as matrix in PowerFlowData #98

merged 8 commits into from
Feb 18, 2025

Conversation

rbolgaryn
Copy link
Contributor

@rbolgaryn rbolgaryn commented Feb 14, 2025 •
edited
Loading

  • add a kwarg calc_loss_factors to ACPowerFlow (default false)
  • write loss factors as an attribute of PowerFlowData - 2D array (rows - bus, columns - time steps)
  • initialize the loss factors as nothing by default
  • test the values of loss factors

@rbolgaryn rbolgaryn added the enhancement New feature or request label Feb 14, 2025
@rbolgaryn rbolgaryn requested a review from GabrielKS February 14, 2025 03:51
@rbolgaryn rbolgaryn self-assigned this Feb 14, 2025
@rbolgaryn rbolgaryn requested a review from jd-lara February 14, 2025 14:41
GabrielKS
GabrielKS requested changes Feb 17, 2025
View reviewed changes
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 code comments. Can you add a description to the pull request for future reference?

@GabrielKS GabrielKS mentioned this pull request Feb 17, 2025
Open
@rbolgaryn
Copy link
Contributor Author

A few code comments. Can you add a description to the pull request for future reference?

Thank you! I have addressed your comments

@rbolgaryn rbolgaryn requested a review from GabrielKS February 17, 2025 23:24
GabrielKS
GabrielKS reviewed Feb 18, 2025
View reviewed changes
Copy link
Contributor

@GabrielKS GabrielKS left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A few more things and then I think this is good from the code quality point of view.

test/test_multiperiod_ac_powerflow.jl
data.bus_activepower_injection .= deepcopy(injs)
data.bus_activepower_withdrawals .= deepcopy(withs)
# allocate timeseries data from csv
prepare_ts_data!(data, time_steps)

# get power flows with NR KLU method and write results
solve_powerflow!(data; pf = pf)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that we have this testset that isn't actually testing anything, but it looks like it was like this before this PR and this PR is just a bit of refactoring, so we can resolve it elsewhere

rbolgaryn and others added 3 commits February 18, 2025 12:50
GabrielKS
GabrielKS reviewed Feb 18, 2025
View reviewed changes
@GabrielKS GabrielKS self-requested a review February 18, 2025 20:45
GabrielKS
GabrielKS approved these changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@GabrielKS GabrielKS left a comment •
edited
Loading

Choose a reason for hiding this comment

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

New bus_type_idx looks good. We should investigate at some point whether it makes sense to do something like NREL-Sienna/PowerSystems.jl#1216 with it (@luke-kiernan has been working on this elsewhere in the PowerFlows codebase), but I'm fine to leave it as future work since this PR doesn't introduce the issue.

@rbolgaryn rbolgaryn merged commit 3cb75e4 into main Feb 18, 2025
6 checks passed
@rbolgaryn rbolgaryn deleted the rb/loss_factors2 branch February 18, 2025 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GabrielKS GabrielKS GabrielKS approved these changes

@jd-lara jd-lara Awaiting requested review from jd-lara

Assignees

@rbolgaryn rbolgaryn

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rbolgaryn @GabrielKS