Sienna-Platform / Sienna-Platform/PowerOperationsModels.jl
Deduplicate the 22 `construct_service!` methods via a shared pipeline
Nobody has claimed this yet.
- Dominant language
- Julia
- Stars
- 2
- Forks
- 1
- Avg merge
- 1d 16h
- Merged PRs (30d)
- 26
Description
Sibling of #172 (construct_device!) and #195 (network-family constructor pairs), for the service side.
Raised by @luke-kiernan reviewing #206:
Maybe have AI do a quick pass on this file for low-hanging fruit for code deduplication.
Pre-existing but theseconstruct_device!calls all look fairly similar...
Deferred out of #206 to keep an already-large refactor reviewable.
Current shape
src/services_models/services_constructor.jl is 943 lines holding 22 construct_service! methods: roughly one per (stage, service type, formulation), split ~500 lines reserve family and ~340 lines transmission interface.
| Formulation | Argument stage | Model stage |
|---|---|---|
RangeReserve |
102 (Reserve), 137 (ConstantReserve) |
171 |
StepwiseCostReserve |
222 | 263 |
GroupReserve |
396 | 413 |
RampReserve |
445 | 478 |
NonSpinningReserve |
523 | 549 |
ConstantMaxInterfaceFlow |
600, 625 | 651, 695, 739 |
VariableMaxInterfaceFlow |
855 | 794, 839, 901 |
The repetition is near-exact, not just thematic
RangeReserve (102-133) and RampReserve (445-476) argument stages are identical apart from the formulation passed to add_service_variables!. Both are:
services = _services_with_contributors(model, sys)
isempty(services) && return
add_parameters!(container, RequirementTimeSeriesParameter, services, model)
for service in services
contributing_devices = get_contributing_devices(model, PSY.get_name(service))
add_service_variables!(container, ActivePowerReserveVariable, service,
contributing_devices, <Formulation>)
add_to_expression!(container, ActivePowerReserveVariable, service, model, devices_template)
add_feedforward_arguments!(container, model, service)
end
NonSpinningReserve (523-547) is the same minus add_to_expression!.
ConstantReserve (137) is the same minus add_parameters!.
Model stages repeat just as closely.
RangeReserve (171-213) and RampReserve (478-521) share the container build, the use_slacks line, the per-service loop, add_to_objective_function!, add_feedforward_constraints! and add_constraint_dual!.
The entire difference is that RampReserve inserts one extra add_constraints!(container, RampConstraint, ...) call.
Proposed direction
A shared pipeline for each stage, with the per-formulation variation expressed as small dispatching predicates rather than as whole copied method bodies.
The file already contains the shape to generalize, at line 215:
_maybe_process_stepwise(container, model,
services::Vector{<:PSY.ReserveDemandTimeSeriesCurve}) =
process_stepwise_cost_reserve_parameters!(container, model, services)
_maybe_process_stepwise(container, model, services) = nothing
Extending that idiom gives roughly:
_maybe_add_requirement_parameters!- no-op forConstantReserve, which has no requirement time series_maybe_add_reserve_expression!- no-op forNonSpinningReserve_maybe_add_ramp_constraints!- active only forRampReserve_maybe_add_participation_constraints!- no-op forStepwiseCostReserve
Each pair is two lines and dispatches on the formulation or service type, so the specialization stays statically resolved and the generic body is written once per stage.
Open questions for whoever picks this up:
- Whether the transmission-interface methods fold into the same pipeline or want their own. They share the
_services_with_contributors/ loop / dual skeleton but differ in nearly everything else, and there are already fiveModelConstructStagemethods there worth looking at on their own. - Whether
GroupReserveparticipates. It is deferred to last inconstruct_services!and reads its inputs from other services rather than from contributing devices, so it may be genuinely separate.
Constraints
- Keep the two-stage
ArgumentConstructStage/ModelConstructStagesplit intact; it is load-bearing for expression-before-constraint ordering. - Run
Test.detect_ambiguitiesafterwards. Collapsing methods onto shared abstract bounds is exactly how ambiguities appear. - No behavior change.
test_services_constructor.jlshould pass untouched.
Worth coordinating with #216, which will also be editing these same method bodies for type stability.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in src/services_models/services_constructor.jl, comparing the 22 construct_service! methods and the existing _maybe_process_stepwise dispatch near line 215. Run test_services_constructor.jl and Test.detect_ambiguities before and after the refactor, coordinating with #216 because it edits the same methods. Done means the two-stage split remains intact, tests pass unchanged, no ambiguities are introduced, and behavior is unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- julia
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100