add tests for lower bound constraints - #149
Conversation
There was a problem hiding this comment.
Pull request overview
Adds unit-test coverage for the lower-bound parameterized range-constraint helpers discussed in issue #130, ensuring the lower-bound wrapper remains correct despite having no in-tree callers today.
Changes:
- Add a comprehensive new test file covering lower/upper bound symmetry, variable vs expression LHS, time-series filtering by ownership, and event-parameter behavior.
- Update mocks to support time-series ownership checks (
IS.has_time_series) and to correctly extendIOM.get_max_active_powerfor mock devices. - Export
add_parameterized_lower_bound_range_constraintsand add docstrings to both direction-specific wrappers.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/verify_mocks.jl | Makes the mock verification use IOM.get_max_active_power explicitly. |
| test/test_parameterized_range_constraints.jl | New unit tests covering lower-bound parameterized range constraints and related edge cases. |
| test/mocks/mock_time_series.jl | Makes MockDeterministic a IS.TimeSeriesData subtype and adds a registry-backed IS.has_time_series for mocks. |
| test/mocks/mock_components.jl | Defines IOM.get_max_active_power methods for mock devices used by event-parameter bounds. |
| test/InfrastructureOptimizationModelsTests.jl | Includes the new parameterized range-constraints test file in the unit test suite. |
| src/InfrastructureOptimizationModels.jl | Exports add_parameterized_lower_bound_range_constraints. |
| src/common_models/range_constraint.jl | Adds docstrings for lower/upper bound wrappers. |
Suppressed comments (1)
src/common_models/range_constraint.jl:458
- Same issue as the lower-bound wrapper: for
P <: EventParameterthe RHS is notmultiplier[name, t] * parameter[name, t](it is scaled byget_max_active_power(device)), so the docstring is misleading for event-parameter bounds.
Add `array[name, t] <= multiplier[name, t] * parameter[name, t]` constraints of type `T`,
where `array` is the variable or expression `U` and the right-hand side comes from the
parameter `P`. For `P <: TimeSeriesParameter` only devices that own the time series named in
`get_time_series_names(model)[P]` are constrained. Mirror of
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| Add `array[name, t] >= multiplier[name, t] * parameter[name, t]` constraints of type `T`, | ||
| where `array` is the variable or expression `U` and the right-hand side comes from the | ||
| parameter `P`. For `P <: TimeSeriesParameter` only devices that own the time series named in | ||
| `get_time_series_names(model)[P]` are constrained. Mirror of | ||
| [`add_parameterized_upper_bound_range_constraints`](@ref). | ||
| """ |
|
Performance Results This branch |
| # Backwards-compatible wrappers | ||
| # Direction-specific wrappers over `add_parameterized_bound_range_constraints`. | ||
| """ | ||
| Add `array[name, t] >= rhs[name, t]` constraints of type `T`, where `array` is the variable or |
There was a problem hiding this comment.
Docstring is identical to the upper_bound version, do they both need it? Could we put just this one docstring on add_parameterized_bound_range_constraints? Or even further: do we need these helpers? A caller could easily pass the LowerBound() or UpperBound() themselves.
There was a problem hiding this comment.
I see the deleted comment says "backwards compatible", but that's probably from PSI.
There was a problem hiding this comment.
Or even further: do we need these helpers? A caller could easily pass the LowerBound() or UpperBound() themselves.
I share your sentiments...but @jd-lara says they're needed and should have tests. See issue #130 for disucssion: after 2 rounds of back-and-forth, I decided to take his word for it, even if it doesn't make sense to me.
| constraint = JuMP.constraint_object(con[name, t]) | ||
| @test constraint.set isa MOI.GreaterThan | ||
| @test JuMP.normalized_rhs(con[name, t]) ≈ multiplier * param_value(name, t) | ||
| @test JuMP.normalized_coefficient(con[name, t], var[name, t]) ≈ 1.0 |
There was a problem hiding this comment.
I think maybe an informative comment here like # Test that we are exercising expression path
But IMO it feels pretty silly to be testing both variable and expression paths. In the code it's just a middle layer that goes either get_variable or get_expression and then passes array to an explicitly unified helper.
(And in line with my musings above, would be a lot simpler if we went straight to the unified helper and dispatched at a smaller level: get_array(container_type))
| end | ||
| end | ||
|
|
||
| @testset "Lower and upper bounds are mirror images" begin |
There was a problem hiding this comment.
This also feels like a silly test, especially since all the helpers are direction-agnostic until you get to the very last _make_bound_constraint which does <= or >= explicitly. So this feels like it's testing 1) did we remember to pass direction 2) are those final helpers pointing the right way.
There was a problem hiding this comment.
And I guess this is part of a broader question, do we want tests for the sake of absolutely ensuring coverage, or do we want to be pruning our tests for importance. I lean towards the latter.
| end | ||
| end | ||
|
|
||
| @testset "Time series parameter constrains only devices owning the time series" begin |
There was a problem hiding this comment.
This does not check that "B" does not have any constraints. Which is fine I think, like I'm not sure we need to test that. But then 1) this test's name is weird and 2) then it's redundant to the other tests, right?
| @test isempty(IOM.get_constraint_keys(container)) | ||
| end | ||
|
|
||
| @testset "Event parameter bounds by the device's max active power" begin |
There was a problem hiding this comment.
Also random thought: I've seen other Julia codebases have much more normalized test names, something like bounds.parameters.events , which makes searching for tests easier (and also works quite nicely with ReTest...)
|
Addressed most of the comments. I cut the tests down some: they still feel mildly overbuilt, but I don't think it's worth spending more time on them. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Address issue #130. Add tests for the lower-bound parameterized range-constraint helpers. (One of these functions--
add_parameterized_lower_bound_range_constraints--looks like dead code to me, but I left it in and gave it a test anyway. See the issue for discussion.)