Fix thermal min-up/down rounding and improve parallel-branch interface error messages#181
Conversation
|
Performance Results
|
There was a problem hiding this comment.
Pull request overview
This PR ports two small fixes from PowerSimulations.jl into PowerOperationsModels.jl: (1) correct rounding for thermal min up/down time conversion in the duration-constraint data path, and (2) improve the diagnostic error message when a transmission interface only partially specifies a reduced parallel/series branch grouping.
Changes:
- Fix
_get_data_for_tdcto callround(x, RoundUp)without an incompatible positionalFloat64argument. - Enhance
_reduced_entry_in_interfaceto include the reduced-entry branch names in the thrownArgumentError.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/static_injector_models/thermal_generation.jl |
Fixes rounding call used when converting time limits into per-resolution step counts for duration constraints. |
src/common_models/add_to_expression.jl |
Improves interface reduction validation errors by listing the branch names involved in the offending reduced entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
luke-kiernan
left a comment
luke-kiernan
left a comment
There was a problem hiding this comment.
Thermal min-up/down rounding — drop the stray Float64 positional argument in round(..., RoundUp) in _get_data_for_tdc (thermal_generation.jl), which is incompatible with the RoundingMode argument.
I'm confused: round(Float64, 1.4, RoundUp) runs just fine for me in REPL. AI hallucination?
| ini_conds[idx, 2] = initial_conditions_off[ix] | ||
| up_val = round(Float64, time_limits.up * steps_per_hour, RoundUp) | ||
| down_val = round(Float64, time_limits.down * steps_per_hour, RoundUp) | ||
| up_val = round(time_limits.up * steps_per_hour, RoundUp) |
There was a problem hiding this comment.
round picks same type as the argument. If time_limits.up and steps_per_hour both are ints, then up_val will also be an int. Check that one or the other is a float so that up_val is consistently a float, not an int
There was a problem hiding this comment.
From PSY docs:
get_time_limits(
value::ThermalStandard
) -> Union{Nothing, @NamedTuple{up::Float64, down::Float64}}So the argument is definitely a float. The change came from Sienna-Platform/PowerSimulations.jl#1527 , so it could've been an AI hallucination there but doesn't feel like it to me.
I'm also unsure why this change was made. It definitely wasn't a method error before, and I would be surprised if there was any other issue.
There was a problem hiding this comment.
You need to revert this. Just leave the improving on erroring message.
There was a problem hiding this comment.
The optimization models take only Floats, it makes no sense to do it with integer
There was a problem hiding this comment.
I left a comment below but replying again here. Since the argument we're passing is always a float, the return type will also be a float. Unless we're anticipating that up or down will change types then this should be safe.
| ini_conds[idx, 2] = initial_conditions_off[ix] | ||
| up_val = round(Float64, time_limits.up * steps_per_hour, RoundUp) | ||
| down_val = round(Float64, time_limits.down * steps_per_hour, RoundUp) | ||
| up_val = round(time_limits.up * steps_per_hour, RoundUp) |
There was a problem hiding this comment.
You need to revert this. Just leave the improving on erroring message.
|
@rodrigomha Sure. Why was this change made in the first place? Are we anticipating that .up will change types? |
The reduced-entry interface check reported that an interface covers only part of a reduced parallel or series group without identifying which branches were involved, leaving no way to locate the offending data.
Two small fixes ported from PSI:
Float64positional argument inround(..., RoundUp)in_get_data_for_tdc(thermal_generation.jl), which is incompatible with theRoundingModeargument.ArgumentError(re-ported ontomain's_error_msg(T)helper).Ported from PSI
The HVDC area-balance fix (#1519) originally bundled with this work is already present on
main, so it is not included here.Part of splitting the former #154 into focused PRs.
🤖 Generated with Claude Code