[Fix] Making SubstituteAccordingTo relies on BOTH the variable ID AND the environment#31
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 87.00% 87.11% +0.10%
==========================================
Files 42 43 +1
Lines 6394 6416 +22
==========================================
+ Hits 5563 5589 +26
+ Misses 734 727 -7
- Partials 97 100 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…not variable was added)
There was a problem hiding this comment.
Pull Request Overview
Fix substitution to respect both variable ID and its originating environment to avoid collisions when variables from different problems share IDs.
- Introduces an Environment interface and a BasicEnvironment implementation with a DefaultEnvironment.
- Extends Variable with an Environment reference and updates constructors and vector/matrix helpers to work with environments.
- Adds tests for SubstituteAccordingTo to ensure environment-aware substitution and an example using vector constraints.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| symbolic/environment.go | Replaces concrete Environment struct with an interface. |
| symbolic/basic_environment.go | Adds BasicEnvironment implementation, MakeBasicEnvironment, and DefaultEnvironment. |
| symbolic/variable.go | Adds Environment field to Variable, updates variable constructors to track env, and changes substitution to consider environment. |
| symbolic/variable_vector.go | Updates function signature to accept Environment interface and uses DefaultEnvironment. |
| symbolic/variable_matrix.go | Updates signatures to accept Environment interface and uses DefaultEnvironment. |
| testing/symbolic/utils_test.go | Updates tests to construct Variables with an Environment. |
| testing/symbolic/variable_test.go | Updates env creation in tests to use MakeBasicEnvironment. |
| testing/symbolic/polynomial_test.go | Adds new test verifying environment-aware substitution. |
| examples/constraints1/vector_constraints.go | New example demonstrating vector constraints. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
symbolic/variable.go
Outdated
| Upper: float64(+Infinity), | ||
| Type: Continuous, | ||
| Name: fmt.Sprintf("x_%v", nextIdx), | ||
| Environment: ¤tEnv, |
There was a problem hiding this comment.
Assigning a pointer to a local interface variable (¤tEnv) embeds a pointer-to-interface whose address differs across constructor calls, breaking intuitive equality of Variable values from the same environment. Once the Environment field is changed to Environment (non-pointer), set this to: Environment: currentEnv.
symbolic/variable.go
Outdated
| Upper: 1.0, | ||
| Type: Binary, | ||
| Name: fmt.Sprintf("x_%v", nextIdx), | ||
| Environment: ¤tEnv, |
There was a problem hiding this comment.
Same issue as above for NewBinaryVariable: avoid storing ¤tEnv (pointer to interface). After changing the field type to Environment, assign the interface value directly: Environment: currentEnv.
| type Environment interface { | ||
| GetName() string | ||
| TrackVariable(v Variable) bool | ||
| AllTrackedVariables() []Variable | ||
| } |
There was a problem hiding this comment.
Changing Environment from a concrete struct to an interface is a breaking API change. Consider adding a migration path: e.g., provide a type alias for the old struct (if applicable), document the change, and add deprecated wrapper constructors/functions that accept *BasicEnvironment to ease transition, or add helper constructors like NewBasicEnvironmentPtr() returning *BasicEnvironment for callers that previously passed *Environment.
Let's try storing the interface directly instead. :) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Remove dereferencing and compare interfaces directly. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… environment objects
Noticed an interesting bug when we are transforming optimization problems: The substitutions needed to represent one problem's variables in terms of another's started to fail when the corresponding variables had matching ID values EVEN if they were from different problems.
This method attempts to address that by making
SubstituteAccordingTodoes replacement based on BOTH: