Ignore units in container keys; add constraint dual keys in a consistent order#109
Merged
Conversation
`_copy_dual_values!` copied dual values positionally (`dual.data .= constraint.data`). When the dual and constraint containers carry the same axis labels in a different order, this misaligns each label's value. This happens for the network `NodalBalanceActiveConstraint`: the dual container is sized from `get_name.(devices)` (component order) while the constraint axis follows the PowerModels bus map order, so each bus's dual was written onto the wrong bus. The permutation is invisible in uncongested hours (flat nodal prices) and only surfaces once congestion makes nodal prices differ — e.g. the DCP-vs-PTDF LMP comparison in PowerOperationsModels. Take the fast broadcast path only when the axes match exactly; otherwise copy by label so values follow their labels regardless of axis order. Also bump PowerNetworkMatrices compat to allow ^0.22. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PSY6 parameterized some component families (e.g. ReserveDemandCurve) on a
unit-system type, so a partially-applied type like ReserveDemandCurve{ReserveUp}
is now a UnionAll rather than a concrete DataType. IS strip_module_name reads
T.parameters (which only exists on DataType), throwing
`FieldError: type UnionAll has no field parameters` whenever such a type appears
in an optimization-container key (e.g. VariableKey for an ORDC reserve variable).
Add _strip_module_name_for_key, which unwraps the UnionAll, keeps the bound
parameters and drops the free TypeVars (so keys stay unique per ReserveUp vs
ReserveDown), and route encode_symbol through it. DataType keys are unchanged.
The proper fix belongs upstream in IS strip_module_name (handle `T isa UnionAll`
like it already handles `T isa Union`); this is a local workaround because IS is
pulled in as a pinned git-branch dependency, not an editable in-repo checkout.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Build the dual container from the existing constraint container's axes so the dual axis matches the constraint exactly, instead of sizing it from IS.get_name.(devices). The PowerModels nodal-balance constraint is built in bus-map order, which differs from component order, so the old positional dual copy wrote each bus's dual onto the wrong bus once congestion made nodal prices differ. _copy_dual_values! now asserts the axes match rather than papering over a mismatch.
PSY6 parameterizes cost-bearing components on a unit system (e.g.
ReserveDemandCurve{ReserveUp, NaturalUnit}). Keys built from a component
instance (typeof(service)) carried the unit parameter while keys built from the
user-declared model type (ReserveDemandCurve{ReserveUp}) did not, so storage and
lookups produced different VariableKey{T,U} objects and lookups failed with
InvalidValue.
Normalize the component type at the single point every container key is
constructed: canonical_component_type drops trailing AbstractUnitSystem
parameters in the OptimizationContainerKey constructors and make_key. It is
@generated so it folds to a compile-time constant (type stable, no runtime
cost), and a no-op for component types without a unit-system parameter.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts optimization container key canonicalization to treat unit-system type parameters as non-identifying, and updates dual-container construction/copying to ensure constraint duals align with the constraint axis ordering.
Changes:
- Canonicalize component types used in
OptimizationContainerKeys by stripping trailing unit-system parameters so keys built from instances and user-declared types match. - Ensure DenseAxisArray dual copying is axis-safe by asserting axes match before positional copying.
- Build network-model dual containers using the existing constraint container’s axis ordering; expand
PowerNetworkMatricescompat to include0.22.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/core/optimization_container_keys.jl | Introduces canonical component-type normalization for container keys (unit-system params ignored). |
| src/core/dual_processing.jl | Adds an axis-equality assertion before positional dual copying for dense containers. |
| src/common_models/add_constraint_dual.jl | Reworks network-model dual container assignment to reuse constraint axes (but currently introduces an order-dependence issue). |
| Project.toml | Expands compat range for PowerNetworkMatrices to include ^0.22. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Performance Results This branch |
jd-lara
reviewed
Jun 3, 2026
jd-lara
requested changes
Jun 3, 2026
Member
jd-lara
left a comment
jd-lara
left a comment
There was a problem hiding this comment.
Clean up excessive AI commentary
jd-lara
reviewed
Jun 5, 2026
| const CONTAINER_KEY_EMPTY_META = "" | ||
|
|
||
| # Strip units before making keys for parametric structs | ||
| @generated function canonical_component_type( |
Member
There was a problem hiding this comment.
we have methods to get this from the key get_component_type
PNM #312 made deepcopy of live solver-factorization caches (KLU/libSparse handles) a deliberate error. Strip PTDF/MODF matrices from the network model before the template deepcopy in the DecisionModel constructor and re-attach the same objects to both the original and the copy. Co-Authored-By: Claude Opus 4.8 (1M context) <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.
No description provided.