Conversation
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
What is the coverage on the ldsda after this change?
AlbertLee125
left a comment
AlbertLee125
left a comment
There was a problem hiding this comment.
I would strongly recommend restoring test_ldsda.py by removing MagicMock.
AlbertLee125
left a comment
AlbertLee125
left a comment
There was a problem hiding this comment.
Return test_ldsda.py back to its original code.
|
There was a bug in the |
|
I think it might make sense to increase LDSDA tests to improve coverage, if that is something we want to do. Before just having a coding agent add new code, let's quantify how much coverage we would increase with these changes to the tests. Moreover, we need to test the bug that is being fixed by adding a new test that, with the previous implementation, would have failed (therefore revealing the bug) and now passes. |
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
I approve this PR with the given changes, I would still want you to address the comments I have made to clean it up and will rerun a copilot review on it to be sure
There was a problem hiding this comment.
Pull request overview
Fixes an LD-SDA implementation bug where line_search() incorrectly handled the tuple return value from _solve_GDP_subproblem(), and adds unit tests to prevent regressions.
Changes:
- Update
GDP_LDSDA_Solver.line_search()to unpack(primal_improved, primal_bound)and use the boolean flag correctly. - Expand
test_ldsda.pywith unit tests that exercise tuple-unpacking behavior and additional internal branches.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
pyomo/contrib/gdpopt/ldsda.py |
Fixes line_search() to correctly unpack _solve_GDP_subproblem() return values; adds/updates docstrings. |
pyomo/contrib/gdpopt/tests/test_ldsda.py |
Adds new unit tests validating tuple-unpacking behavior and exercising additional LD-SDA branches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
Besides, this PR is failing the lint test, please follow the instructions here regarding black https://pyomo.readthedocs.io/en/stable/contribution_guide.html |
Removed the documentation from the class docstring. Co-authored-by: Copilot <[email protected]>
capitalized the notations. Co-authored-by: Copilot <[email protected]>
Refine test class docstring to clarify focus on critical control-flow and validation.
Refine test description for clarity and focus.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* the test docstring/comment no longer references specific source line numbers.
* unused test attribute (self.number_of_external_variables) removed.
2. Changed ldsda.py:
* update the wording to “squared Euclidean distance” in the comment describing the tiebreaker logic.
The current lint test problems come from other files
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ailure Previously, primal_bound was computed even when the solver returned an unsuccessful termination condition. Now primal_bound is only computed when primal_improved is True, making the behavior consistent with the docstring that states primal_bound is None when the subproblem is infeasible.
Replace tabulate-based reformulation summary with simple logger output, consistent with other GDPopt solvers. This avoids adding an unnecessary dependency to the package.
|
Hi — I pushed two fixes to the fork branch
All The branch with the fixes is: https://github.com/SECQUOIA/pyomo/tree/pr-2 Could you pull these changes into your |
|
I also created a PR directly into the branch that sets up this one, if you merge that PR, we should be good to go Toflamus#1 |
Removed the tabulate dependency and replaced it with simple logger output. Fixed _solve_GDP_subproblem so primal_bound is None when the solver fails (not just on preprocessing infeasibility).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Address review feedback: use LinearRepnVisitor and fix var_name_dict
Fixes # .
Summary/Motivation:
Description
Fixed a bug in ldsda.py where the line_search method failed to correctly unpack the tuple return value from _solve_GDP_subproblem.
Testing
Ran pytest locally on the new test file.
Verified code coverage passes requirements.
Formatted code using black.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: