-
Notifications
You must be signed in to change notification settings - Fork 6
CollocatedIntegratedOptimizationProblem: Add option to disable initial constraints #1704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…l constraints Initial constraints are only required when an incomplete set of initial states is provided. If the set of initial states is either complete or "sufficiently" complete (e.g., values of all differentiated states are provided), then initial constraints may or may not be required depending on the use case. This change introduces an option that controls explicitly whether or not the initial constraints are created.
|
@jbaayen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jbaayen ,
Thank you for this pull request, which extends the flexibility of the model specification in RTC-Tools.
Please consider adding validation to ensure that when add_initial_constraints=False, all initial states must be fully specified.
When add_initial_constraints=True and all initial states are specified, a warning could be issue to inform the user that the system may be overdetermined.
Please note that some conflicts in the test_modelica_mixin file need to be resolved.
| :cvar check_collocation_linearity: | ||
| If ``True``, check whether collocation constraints are linear. Default is ``True``. | ||
| :cvar add_initial_constraints: | ||
| If ``True``, add constraints for initial conditions. Default is ``True``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be a bit more concise:
If True (default), add constraints for initial conditions
| any model equations that are collocated must be linear. By default, all | ||
| model equations are collocated, and linearity of the model equations is | ||
| verified. Working with non-linear models is possible, but discouraged. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may sound redundant, but adding a note like this one—or one that's a bit shorter—might be helpful:
When the initial states are fully specified, you can disable the initial constraints by setting the class variable add_initial_constraints to False when constructing the problem. Only disable initial constraints if all initial state variables have been fully and explicitly specified as part of the problem setup (e.g., through initial conditions or history). If any initial state is missing and initial constraints are disabled, the problem may become underdetermined, and the optimization may produce incorrect results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds an option to disable initial constraints in the CollocatedIntegratedOptimizationProblem class. This allows users to control whether initial constraints are created, which is useful when a complete set of initial states is provided and initial constraints may lead to an overdetermined system.
- Add
add_initial_constraintsclass variable to control initial constraint generation - Conditionally execute initial constraint-related code based on this flag
- Add comprehensive tests to verify both enabled and disabled behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/rtctools/optimization/collocated_integrated_optimization_problem.py | Implements the core functionality by adding the add_initial_constraints flag and wrapping constraint generation logic with conditional checks |
| tests/optimization/test_modelica_mixin.py | Adds test cases to verify the new functionality works correctly with and without initial constraints |
Comments suppressed due to low confidence (1)
src/rtctools/optimization/collocated_integrated_optimization_problem.py:1
- The comment reformatting appears to be unintentional. The original single-line comment on line 614 should be kept instead of being split and duplicated across lines 615-616.
import itertools
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| init_der = (h.values[-1] - h.values[-2]) / ( | ||
| h.times[-1] - h.times[-2] | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line break formatting change appears unrelated to the PR's purpose. The original single-line format should be preserved to maintain consistency with the existing codebase.
| init_der = (h.values[-1] - h.values[-2]) / ( | |
| h.times[-1] - h.times[-2] | |
| ) | |
| init_der = (h.values[-1] - h.values[-2]) / (h.times[-1] - h.times[-2]) |
Copilot uses AI. Check for mistakes.
| :cvar check_collocation_linearity: | ||
| If ``True``, check whether collocation constraints are linear. Default is ``True``. | ||
| :cvar add_initial_constraints: | ||
| If ``True``, add constraints for initial conditions. Default is ``True``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it also be made explicit to which variables this refers to: both constant and controlled inputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and useful feature. One small request to make clear in documentation that all initial conditions will not be picked up and added as constraints.
|
After resolving the conflict and addressing the previous comments, this merge can be completed. |
|
Currently SonarCloud Quality Gate failes due to test coverage. It seems that SonarCloud needs to read the actual coverage report file when it runs the scan. If the file is not present during the SonarCloud scan job, it will default to 0% coverage, regardless of what the logs show. |
Initial constraints are only required when an incomplete set of initial states is provided. If the set of initial states is either complete or "sufficiently" complete (e.g., values of all differentiated states are provided), then initial constraints may or may not be required depending on the use case.
This change introduces an option that controls explicitly whether or not the initial constraints are created.