Improvements to LDSDA by bernalde · Pull Request #3830 · Pyomo/pyomo

bernalde · 2026-02-02T19:57:06Z

Fixes # .

Summary/Motivation:

A bug in ldsda.py was found related to unpacking a tuple
Code coverage of ldsda.py was not 100%
Documentation and formatting were lacking in this file
An extra dependency (tabulate) was unnecessarily added to the code

Changes proposed in this PR:

Modified line_search in pyomo/contrib/gdpopt/ldsda.py to unpack (primal_improved, primal_bound).
Added unit tests in pyomo/contrib/gdpopt/tests/test_ldsda.py to verify the logic handles tuple returns correctly without infinite loops and raises coverage of ldsda.py to 100%
Make behavior consistent with the docstring for _solve_GDP_subproblem: ensure primal_bound is None when the subproblem solve does not succeed (not only when preprocessing raises InfeasibleConstraintException).
Avoid adding an unnecessary global dependency: remove use of the external tabulate package from ldsda and use simple logger output instead.

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:

I agree my contributions are submitted under the BSD license.
I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

… units

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Removed the documentation from the class docstring. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

capitalized the notations. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Refine test class docstring to clarify focus on critical control-flow and validation.

Refine test description for clarity and focus.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* 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.

…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.

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).

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Fix/ldsda fix

blnicho · 2026-02-03T19:09:42Z

@bernalde could you give the Pyomo team edit permissions on this PR? Should be a checkbox at the bottom of the right side information summary about "Allowing maintainers to edit this pull request".

bernalde · 2026-02-03T19:46:45Z

Oh no, since this is coming from the SECQUOIA fork I can’t do that https://github.com/orgs/community/discussions/5634
Should I open a PR from my personal account?

emma58

Thank you for fixing the bug and adding so many tests! I put several comments in, but nothing needs to hold up the PR other than that I think you are missing assertions in one test, and there's on import that is inside of a test. The rest is all just preference and some design ideas that are totally optional.

pyomo/contrib/gdpopt/tests/test_ldsda.py

emma58 · 2026-02-03T19:57:10Z

pyomo/contrib/gdpopt/tests/test_ldsda.py

+        with self.assertRaisesRegex(
+            ValueError, "should be a list of ExactlyExpression"
+        ):
+            self.solver._get_external_information(self.model.util_block, self.config)


Same question about testing private methods here (and in the tests following as well.)

pyomo/contrib/gdpopt/tests/test_ldsda.py

pyomo/contrib/gdpopt/ldsda.py

blnicho · 2026-02-03T20:46:33Z

Oh no, since this is coming from the SECQUOIA fork I can’t do that https://github.com/orgs/community/discussions/5634 Should I open a PR from my personal account?

Ok, no problem. I didn't know about that limitation. We'll work with this PR, no need to reopen it from a personal fork.

…s, make get_external_information public - Add DirectionNorm enum (L2, Linf) with backward compatibility for string values - Add SearchPhase enum (INITIAL, NEIGHBOR, LINE) for search type - Rename _get_external_information to get_external_information (public API) - Update all usages and tests accordingly

codecov · 2026-02-04T05:11:52Z

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.75%. Comparing base (89ebf89) to head (1f63298).
⚠️ Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
pyomo/contrib/gdpopt/ldsda.py	96.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3830      +/-   ##
==========================================
+ Coverage   89.73%   89.75%   +0.01%     
==========================================
  Files         904      904              
  Lines      105870   105885      +15     
==========================================
+ Hits        95005    95035      +30     
+ Misses      10865    10850      -15

Flag	Coverage Δ
builders	`29.04% <40.74%> (+0.01%)`	⬆️
default	`83.87% <96.29%> (?)`
expensive	`35.50% <40.74%> (?)`
linux	`86.92% <66.66%> (-2.56%)`	⬇️
linux_other	`86.92% <66.66%> (+0.08%)`	⬆️
oldsolvers	`29.87% <40.74%> (-0.01%)`	⬇️
osx	`83.09% <66.66%> (+0.08%)`	⬆️
win	`85.15% <66.66%> (+0.08%)`	⬆️
win_other	`85.15% <66.66%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

emma58

A couple comments on the change to enums, but otherwise this looks good to me!

emma58 · 2026-02-10T19:33:37Z

pyomo/contrib/gdpopt/ldsda.py

+            The dimensionality of the neighborhood (number of external variables).
        config : ConfigBlock
-            GDPopt configuration block
+            The configuration block specifying the norm ('L2' or 'Linf').


You probably want to update this docstring to reference your new DirectionNorm enum.

My apologies for the late response. It is a good point. Since DirectionNorm is a str-Enum, the legacy == 'L2'/'Linf' checks would work with either enum or string inputs. The explicit tuple membership check was retained mainly to make it visually clear that both DirectionNorm values and legacy string inputs are accepted. I am happy to simplify it if you'd prefer.

I personally think simplifying it is better, and that you should encourage users to use the enum rather than relying on string arguments. We get burned relying on strings a lot and have been trying to move away from it. But I'm not opposed to this going in as is if that's your preference.

(Though I do think you may as well make the reversions in the comments below to just check the equality like before.)

emma58 · 2026-02-10T19:34:12Z

pyomo/contrib/gdpopt/ldsda.py

-        if config.direction_norm == 'L2':
+        if config.direction_norm in (DirectionNorm.L2, 'L2'):


Since you set the value of the enum to be the string, the old version should work in either case.

Since DirectionNorm is a str-Enum, another clean option would be to normalize once using norm = str(config.direction_norm) and then compare against the string values. This keeps compatibility with legacy string inputs while avoiding redundant membership checks.
Something like,

norm = str(config.direction_norm) if norm == 'L2': ... elif norm == 'Linf': ...

You don't need to cast it to a string--it's already a string. You can just make this line:

if config.direction_norm == DirectionNorm.L2

(This works because DirectionNorm.L2 == 'L2')

(Sorry, just realized my original comment was confusing. What I meant is above.)

emma58 · 2026-02-10T19:34:52Z

pyomo/contrib/gdpopt/ldsda.py

-        elif config.direction_norm == 'Linf':
+        elif config.direction_norm in (DirectionNorm.Linf, 'Linf'):


Same comment here: Since you set the value of the enum to be the string, the old version should work in either case.

If normalizing the DirectionNorm using norm = str(config.direction_norm) is a good idea, I will implement it.

blnicho · 2026-02-12T23:40:44Z

@bernalde or @AlbertLee125 could you reply to Emma's review comments or mark them resolved if there isn't anything to be done?

AlbertLee125 · 2026-02-13T01:40:56Z

My apologies for the late response. It is a good point. Since DirectionNorm is a str-Enum, the legacy == 'L2'/'Linf' checks would work with either enum or string inputs. The explicit tuple membership check was retained mainly to make it visually clear that both DirectionNorm values and legacy string inputs are accepted. I am happy to simplify it if you'd prefer.

AlbertLee125 · 2026-02-13T18:23:13Z

I applied the suggested change. Backward compatibility remains intact. Thank you very much for your kind instruction.

blnicho · 2026-02-13T20:10:12Z

pyomo/contrib/gdpopt/tests/test_ldsda.py

-from pyomo.environ import (
-    SolverFactory,
-    value,
-    Var,
-    Constraint,
-    TransformationFactory,
-    ConcreteModel,
-    BooleanVar,
-    LogicalConstraint,
-    Block,
-)


@AlbertLee125 this looks like a bad merge. You lost some of the imports required for this test.

Thanks for pointing that out. It was a merge issue on my side. I restored the missing imports.

blnicho · 2026-02-14T05:41:40Z

Jenkins failure is unrelated (it's an intermittent cuOpt failure). This is safe to merge.

Toflamus and others added 19 commits January 27, 2026 19:12

Fix the bug of linear search function in LDSDA implementation

05eae5c

Reformattd with black package and add testing codes for linear search…

8e24017

… units

Fix tuple unpacking bug in ldsda.py line_search and add unit test

d7840d4

Reformatted with latest black

d3f5409

build more tests and add the documentation

286d9df

Update pyomo/contrib/gdpopt/tests/test_ldsda.py

2e32366

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update pyomo/contrib/gdpopt/ldsda.py

a8115cd

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Apply suggestion from @Copilot

2b683fb

Removed the documentation from the class docstring. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update pyomo/contrib/gdpopt/tests/test_ldsda.py

8be8b92

capitalized the notations. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update docstring for TestLDSDAUnits class

c85af0b

Refine test class docstring to clarify focus on critical control-flow and validation.

Update test description for TestLDSDAUnits

50dfc5e

Refine test description for clarity and focus.

Update pyomo/contrib/gdpopt/tests/test_ldsda.py

eb79c60

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Black formatting ldsda.py

95c7119

Remove tabulate dependency from ldsda.py

cc2812e

Replace tabulate-based reformulation summary with simple logger output, consistent with other GDPopt solvers. This avoids adding an unnecessary dependency to the package.

Merge pull request #1 from SECQUOIA/pr-2

e7dc490

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).

Update pyomo/contrib/gdpopt/tests/test_ldsda.py

d160da7

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Merge pull request #2 from Toflamus/fix/ldsda-fix

e20f14c

Fix/ldsda fix

blnicho added this to Pyomo 6.10 Feb 3, 2026

github-project-automation bot moved this to Todo in Pyomo 6.10 Feb 3, 2026

Merge branch 'main' into main

094ea54

emma58 reviewed Feb 3, 2026

View reviewed changes

emma58 added the AT: PRE-TEST INSPECTED label Feb 3, 2026

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Feb 3, 2026

bernalde added 2 commits February 3, 2026 15:47

Running black

6436801

Address PR review comments: move import to top, remove inline comment

76c516c

blnicho moved this from Todo to Review In Progress in Pyomo 6.10 Feb 4, 2026

emma58 added the AT: PRE-TEST INSPECTED label Feb 4, 2026

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Feb 4, 2026

Merge branch 'main' into main

2fc5b24

emma58 approved these changes Feb 10, 2026

View reviewed changes

github-project-automation bot moved this from Review In Progress to Reviewer Approved in Pyomo 6.10 Feb 10, 2026

emma58 added the AT: PRE-TEST INSPECTED label Feb 10, 2026

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Feb 10, 2026

Merge branch 'Pyomo:main' into main

0b77923

blnicho moved this from Reviewer Approved to Review In Progress in Pyomo 6.10 Feb 11, 2026

Merge branch 'Pyomo:main' into main

9bc5697

AlbertLee125 added 2 commits February 12, 2026 23:26

Refactor direction_norm checks for L2 and Linf

064d9b6

Merge branch 'main' into main

56e398d

blnicho reviewed Feb 13, 2026

View reviewed changes

Refactor imports in test_ldsda.py and add missing GDP components.

1f63298

blnicho approved these changes Feb 14, 2026

View reviewed changes

github-project-automation bot moved this from Review In Progress to Reviewer Approved in Pyomo 6.10 Feb 14, 2026

blnicho added the AT: PRE-TEST INSPECTED label Feb 14, 2026

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Feb 14, 2026

blnicho merged commit 634a186 into Pyomo:main Feb 14, 2026
34 of 35 checks passed

github-project-automation bot moved this from Reviewer Approved to Done in Pyomo 6.10 Feb 14, 2026

		if config.direction_norm == 'L2':
		if config.direction_norm in (DirectionNorm.L2, 'L2'):

		elif config.direction_norm == 'Linf':
		elif config.direction_norm in (DirectionNorm.Linf, 'Linf'):

Comments

Conversation

bernalde commented Feb 2, 2026

Fixes # .

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

Uh oh!

blnicho commented Feb 3, 2026

Uh oh!

bernalde commented Feb 3, 2026

Uh oh!

emma58 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blnicho commented Feb 3, 2026

Uh oh!

codecov bot commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

emma58 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emma58 Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blnicho commented Feb 12, 2026

Uh oh!

AlbertLee125 commented Feb 13, 2026

Uh oh!

AlbertLee125 commented Feb 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blnicho commented Feb 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Feb 4, 2026 •

edited

Loading

emma58 Feb 13, 2026 •

edited

Loading