Add hydro formulation with bilinear approximation#87
Open
acostarelli wants to merge 4 commits into
Open
Conversation
acostarelli
changed the title
acostarelli
force-pushed
the
ac/hydro-bilinear
branch
from
282a341 to
bf6035d
Compare
|
Performance Results
|
luke-kiernan
requested changes
Apr 24, 2026
Collaborator
There was a problem hiding this comment.
More so than the below comment, I'd like to see one or two unit tests. IOM tests the formulation math, so you don't need to do that. The concern is more so: does the POM plumbing hand things to IOM correctly? E.g. spot check a couple constraints and coefficients for a simple case, to confirm that things are getting matched up as intended, that we're not missing a unit conversion somewhere
Comment on lines
+1853
to
+1856
| ( | ||
| min = get_variable_lower_bound(HydroTurbineFlowRateVariable, d, W), | ||
| max = get_variable_upper_bound(HydroTurbineFlowRateVariable, d, W), | ||
| ) for _ in 1:length(reservoirs) |
Collaborator
There was a problem hiding this comment.
Oh, you're passing in the same bounds for all the reservoirs here. You could make that clearer via repeat.
It took me a bit of thought to work out the shapes: might be worth mentioning that we're assuming min/max bounds are time-independent here.
Member
Author
There was a problem hiding this comment.
Do min/max bounds change over time in other cases?
Collaborator
There was a problem hiding this comment.
Time-varying max_active_power is super common, e.g. for solar or wind. I can't find a spot where head height or outflow limits are time-varying, though.
luke-kiernan
approved these changes
Apr 28, 2026
Collaborator
luke-kiernan
left a comment
luke-kiernan
left a comment
There was a problem hiding this comment.
My other 2 cents (more of a nitpick): the commit situation is 10x more complex than the code merits...I'd be tempted to merge from a fresh branch with just 1 or 2 commits.
| (hydro_vol_df[1, :value]) + | ||
| ((total_inflow - total_outflow - total_spillage) * 3600 * 1e-9) | ||
|
|
||
| @test abs(calculated_vf - hydro_vol_df[end, :value]) <= 1e-4 |
Collaborator
There was a problem hiding this comment.
This looks like a good use case for approx. Might be worth commenting why 1e-4 is reasonable: presumably calculated_vf is signficiantly larger? Some magic numbers in calculate_vf too.
Member
Author
There was a problem hiding this comment.
I copied this test from the other Bilinear formulation tbh
Contributor
There was a problem hiding this comment.
I agree with Luke here. I would replace the test by:
@test isapprox(calculated_vf, hydro_vol_df[end, :value], atol = tol
In the same sense, can you define tol = 1e-4 and a comment to why using 1e-4?
| repeat( | ||
| [ | ||
| ( | ||
| min = get_variable_lower_bound(HydroTurbineFlowRateVariable, d, W), |
Collaborator
There was a problem hiding this comment.
nitpick: might be worth defining a function for this (variable, device, formulation_type) -> (min = ..., max = ...) pattern. Only 2 spots right now (so probably not), but will new PRs add more?
Adds the bilinear (flow*head) hydro dispatch formulation and folds in adjacent refactors merged through this branch: - Hydro and storage updates to IOM helpers (#97) - POM-to-IOM type-dispatch API migration - MarketBidCost / ImportExportCost static/TS split + IEC refactor - Shiftable-load interval indexing and validation fixes - HDF system serialization (#75) - Pin GitHub revisions; bridge IOM system-query stubs to PSY public API Co-Authored-By: Luke Kiernan <86331877+luke-kiernan@users.noreply.github.com> Co-Authored-By: Rodrigo Henríquez-Auba <rodrigo.henriquezauba@nrel.gov> Co-Authored-By: Jose Daniel Lara <jdlara@berkeley.edu> Co-Authored-By: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
acostarelli
force-pushed
the
ac/hydro-bilinear
branch
from
9d2e700 to
0bf1ff5
Compare
Contributor
|
@acostarelli Can you fix the conflicts to use what should remain? You can probably resolve the conflicts in the web |
Member
Author
|
@copilot resolve the merge conflicts in this pull request |
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
# Conflicts: # src/PowerOperationsModels.jl # src/static_injector_models/hydro_generation.jl # src/static_injector_models/hydrogeneration_constructor.jl
acostarelli
force-pushed
the
ac/hydro-bilinear
branch
from
68766db to
f559b20
Compare
rodrigomha
reviewed
May 13, 2026
|
|
||
| calculated_vf = | ||
| (hydro_vol_df[1, :value]) + | ||
| ((total_inflow - total_outflow - total_spillage) * 3600 * 1e-9) |
Contributor
There was a problem hiding this comment.
Rather than using 3600 and 1e-9, can you use POM.SECONDS_IN_HOUR and POM.M3_TO_KM3, as defined here: https://github.com/Sienna-Platform/PowerOperationsModels.jl/blob/main/src/core/physical_constant_definitions.jl
rodrigomha
requested changes
May 13, 2026
Contributor
rodrigomha
left a comment
rodrigomha
left a comment
There was a problem hiding this comment.
Just minor comments in the tests for readability and trying to be consistent. After those changes we should be ready to merge.
Use POM.SECONDS_IN_HOUR / POM.M3_TO_KM3 in the m^3/s -> km^3 water-balance conversion instead of bare 3600 / 1e-9 literals, and switch the volume-closure check to isapprox(...; atol = tol) with a named tolerance and a brief comment on its sizing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requires IOM on : Sienna-Platform/InfrastructureOptimizationModels.jl#86