Support multiple group reserve services and add tests#1607
Support multiple group reserve services and add tests#1607ChenHaotianC wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the service construction pipeline to support multiple GroupReserve service models within the same ProblemTemplate/model build, and adds a regression test to ensure multiple reserve groups produce distinct requirement constraints.
Changes:
- Update
construct_services!to defer allGroupReserveservice models (not just one) until the end of service construction. - Add a new test that builds a model with two
ConstantReserveGroupservices and verifies each group’s requirement constraint RHS. - Change contributing-device population behavior for empty mappings from
errorto@warn+continue.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/services_models/services_constructor.jl |
Collect and construct multiple GroupReserve services at the end of service construction. |
test/test_services_constructor.jl |
Add test coverage for multiple ConstantReserveGroup constraints with different requirements. |
src/services_models/reserves.jl |
Alters NonSpinning reserve logic by switching from time_limits.up to time_limits.down (appears incorrect). |
src/operation/problem_template.jl |
Downgrade empty contributing-device mapping from hard error to warning/skip. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| groupservices = Any[] | ||
|
|
||
| for (key, service_model) in services_template | ||
| if get_formulation(service_model) === GroupReserve # group service needs to be constructed last | ||
| groupservice = key | ||
| if get_formulation(service_model) === GroupReserve # group service needs to be constructed last | ||
| push!(groupservices, key) | ||
| continue |
There was a problem hiding this comment.
This is okay I guess? @ChenHaotianC You could try to replace with something like:
groupservices = Tuple{String, Symbol}[] and check if its works?
| groupservices = Any[] | ||
| for (key, service_model) in services_template | ||
| if get_formulation(service_model) === GroupReserve # group service needs to be constructed last | ||
| groupservice = key | ||
| if get_formulation(service_model) === GroupReserve # group service needs to be constructed last | ||
| push!(groupservices, key) | ||
| continue |
| component_type = typeof(d) | ||
| name = PSY.get_name(d) | ||
| varstatus = get_variable(container, OnVariable(), component_type) | ||
| startup_time = PSY.get_time_limits(d).up | ||
| startup_time = PSY.get_time_limits(d).down | ||
| ramp_limits = _get_ramp_limits(d) | ||
| if reserve_response_time > startup_time |
There was a problem hiding this comment.
I think this is somewhat correct. The time_limits(d).up is how much hours the unit needs to be kept on once the OnVariable changes from 0 to 1. It is what is called the minimum start up time time.
In here we are using this as how long it takes to be completely turn on. @jd-lara Is it okay to use these two approaches interchangeably here?
| error( | ||
| "The contributing devices for service $(PSY.get_name(service)) is empty. Add contributing devices to the service in the data to continue.", | ||
| ) | ||
| @warn "The contributing devices for service $(PSY.get_name(service)) is empty, consider removing the service from the system" _group = |
There was a problem hiding this comment.
@ChenHaotianC accept this change. This is correct.
rodrigomha
left a comment
rodrigomha
left a comment
There was a problem hiding this comment.
@ChenHaotianC Fix the minor changes here and fix the conflict from git.
@jd-lara We need to figure out how we are going to distinguish min up/down times with how long it takes a unit to ramp-up.
Adds support for multiple GroupReserve services in the same model and includes tests for the updated behavior.