Add FlowSign trait for variable directionality#98
Open
luke-kiernan wants to merge 2 commits into
Open
Conversation
Holy-traits style: `flow_sign(V)` returns Injection/Withdrawal/Unsigned; `multiplier_from_sign` resolves ±1.0 via dispatch. Lets POM derive most power-balance multipliers from the variable type alone. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a flow-direction “trait” layer to encode the sign convention (injection vs withdrawal) associated with variable types, aiming to replace scattered sign/multiplier overrides and centralize directionality logic.
Changes:
- Introduces
FlowSigntrait types (Injection,Withdrawal,Unsigned) plusflow_signandmultiplier_from_signhelpers. - Defines default and standard mappings for active power variable types.
- Removes an outdated “not included” note from rate-of-change constraints.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/InfrastructureOptimizationModels.jl |
Exports the new flow-direction trait API. |
src/core/standard_variables_expressions.jl |
Defines FlowSign trait types, default behavior, and mappings for standard power variables. |
src/common_models/rateofchange_constraints.jl |
Removes an obsolete header note. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+55
to
+64
| struct Unsigned <: FlowSign end | ||
|
|
||
| # Default: no directional meaning attached to the variable type. | ||
| flow_sign(::Type{<:VariableType}) = Unsigned | ||
|
|
||
| multiplier_from_sign(::Type{Injection}) = 1.0 | ||
| multiplier_from_sign(::Type{Withdrawal}) = -1.0 | ||
| # Variables without flow semantics (OnVariable, StartVariable, ...) keep the | ||
| # legacy 1.0 default so callers that don't care about sign still work. | ||
| multiplier_from_sign(::Type{Unsigned}) = 1.0 |
| export RateofChangeConstraintSlackUp, RateofChangeConstraintSlackDown | ||
| export DCVoltage | ||
| # Flow-direction trait for variable types | ||
| export FlowSign, Injection, Withdrawal, Unsigned |
Comment on lines
+57
to
+69
| # Default: no directional meaning attached to the variable type. | ||
| flow_sign(::Type{<:VariableType}) = Unsigned | ||
|
|
||
| multiplier_from_sign(::Type{Injection}) = 1.0 | ||
| multiplier_from_sign(::Type{Withdrawal}) = -1.0 | ||
| # Variables without flow semantics (OnVariable, StartVariable, ...) keep the | ||
| # legacy 1.0 default so callers that don't care about sign still work. | ||
| multiplier_from_sign(::Type{Unsigned}) = 1.0 | ||
|
|
||
| # Standard variable types defined here: | ||
| flow_sign(::Type{ActivePowerVariable}) = Injection | ||
| flow_sign(::Type{ActivePowerInVariable}) = Withdrawal | ||
| flow_sign(::Type{ActivePowerOutVariable}) = Injection |
Contributor
|
Performance Results This branch |
Address Copilot review on #98: rename Injection/Withdrawal/Unsigned to FlowInjection/FlowWithdrawal/FlowUndirected to avoid the name clash with Base.Unsigned and reduce ambiguity for downstream `using` consumers. Add test/test_flow_sign.jl covering defaults, standard mappings, and multiplier roundtrip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
|
Flows and injection directions aren't the same ... I am not sure this design is sound |
Collaborator
Author
Is there some other name you'd suggest? Goals here were:
|
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.
Cleanup
get_variable_multiplier. Stacked on top of #97 for convenience, but I'd just merge #97 first, then this.