Pyomo.DoE: Corrected initialization when using only lower diagonal of FIM by adowling2 · Pull Request #3532 · Pyomo/pyomo

adowling2 · 2025-03-22T00:59:38Z

Fixes # .

Summary/Motivation:

Pyomo.DoE did not correctly initialize the Cholesky factorization when tracking only the lower diagonal of the FIM
Found the bug due to a Cholesky factorization failure, which led to printing out the FIM used for initialization

Changes proposed in this PR:

Now correctly computing the full FIM for initialization
Also checking the smallest eigenvalue of the prior FIM and added jitter if needed to ensure FIM used for initialization is positive definite (and thus the Cholesky factorization is more reliable)
Also checking the provided prior is symmetric

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.

adowling2 · 2025-03-22T01:00:06Z

@djlaky Check this out

adowling2 · 2025-03-23T23:08:25Z

@mrmundt @blnicho This is ready for a (hopefully quick) review.

mrmundt

I will never not love that "jitter" is a real academically used term. This looks good but I suggest adding a test to ensure behavior.

blnicho

I agree with Miranda about adding a test but otherwise this looks fine.

codecov · 2025-03-24T16:15:09Z

Codecov Report

Attention: Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.70%. Comparing base (fa15cc7) to head (0fa9c2b).
Report is 763 commits behind head on main.

Files with missing lines	Patch %	Lines
pyomo/contrib/doe/doe.py	93.33%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3532   +/-   ##
=======================================
  Coverage   88.70%   88.70%           
=======================================
  Files         888      888           
  Lines      102024   102037   +13     
=======================================
+ Hits        90505    90517   +12     
- Misses      11519    11520    +1

Flag	Coverage Δ
builders	`26.56% <13.33%> (+<0.01%)`	⬆️
default	`84.82% <93.33%> (?)`
expensive	`33.96% <13.33%> (?)`
linux	`86.18% <93.33%> (-2.28%)`	⬇️
linux_other	`86.18% <93.33%> (+<0.01%)`	⬆️
osx	`76.09% <46.66%> (-0.01%)`	⬇️
win	`84.66% <93.33%> (+<0.01%)`	⬆️
win_other	`84.66% <93.33%> (+<0.01%)`	⬆️

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.

adowling2 · 2025-03-24T18:16:36Z

jitter = the FIM is not awake yet and needs some coffee ;)

I'll work on a test

… question that we should probably have some extra error checking

adowling2 · 2025-03-24T18:42:13Z

TODO:

Add an error message if the prior is significantly negative definite (i.e., smallest eigenvalue is less than 1E-6)
Test said error message
Add an error message for if the prior is not symmetric (with a test)
Update the test for the jitter
Run Black

Still need to check the tests work

adowling2 · 2025-03-24T21:53:54Z

TODO:

Check that new tests runs correctly

adowling2 · 2025-03-25T22:11:26Z

I have one test failing on my local machine with the message:

>       with self.assertRaisesRegex(
            ValueError,
            "FIM provided is not positive definite. It has one or more negative eigenvalue(s) less than -{:.1e}".format(
                    _SMALL_TOLERANCE_DEFINITENESS
            ),
        ):
E       AssertionError: "FIM provided is not positive definite. It has one or more negative eigenvalue(s) less than -1.0e-06" does not match "FIM provided is not positive definite. It has one or more negative eigenvalue(s) less than -1.0e-06"

tests/test_doe_errors.py:184: AssertionError

Any insights are appreciated. I'm concerned that even if this passed with GitHub Actions, I am introducing a fragile test.

…wling2/pyomo into pyomo-doe-fix-initialization

adowling2 · 2025-03-26T12:06:01Z

@jsiirola @blnicho @mrmundt Any ideas on how to debug this test? Here is the output:

=================================== FAILURES ===================================
__ TestReactorExampleErrors.test_reactor_check_bad_prior_negative_eigenvalue ___
ValueError: FIM provided is not positive definite. It has one or more negative eigenvalue(s) less than -1.0e-06

During handling of the above exception, another exception occurred:

self = <pyomo.contrib.doe.tests.test_doe_errors.TestReactorExampleErrors testMethod=test_reactor_check_bad_prior_negative_eigenvalue>

    def test_reactor_check_bad_prior_negative_eigenvalue(self):
        from pyomo.contrib.doe.doe import _SMALL_TOLERANCE_DEFINITENESS
    
        fd_method = "central"
        obj_used = "trace"
        flag_val = 0  # Value for faulty model build mode - 0: full model
    
        prior_FIM = -np.ones((4, 4))
    
        experiment = FullReactorExperiment(data_ex, 10, 3)
    
        DoE_args = get_standard_args(experiment, fd_method, obj_used, flag_val)
        DoE_args['prior_FIM'] = prior_FIM
    
        doe_obj = DesignOfExperiments(**DoE_args)
    
        with self.assertRaisesRegex(
            ValueError,
            "FIM provided is not positive definite. It has one or more negative eigenvalue(s) less than -{:.1e}".format(
                _SMALL_TOLERANCE_DEFINITENESS
            ),
        ):
>           doe_obj.create_doe_model()
E           AssertionError: "FIM provided is not positive definite. It has one or more negative eigenvalue(s) less than -1.0e-06" does not match "FIM provided is not positive definite. It has one or more negative eigenvalue(s) less than -1.0e-06"

pyomo/contrib/doe/tests/test_doe_errors.py:190: AssertionError
------- generated xml file: /home/runner/work/pyomo/pyomo/TEST-pyomo.xml -------
=========================== short test summary info ============================
FAILED pyomo/contrib/doe/tests/test_doe_errors.py::TestReactorExampleErrors::test_reactor_check_bad_prior_negative_eigenvalue - AssertionError: "FIM provided is not positive definite. It has one or more negative eigenvalue(s) less than -1.0e-06" does not match "FIM provided is not positive definite. It has one or more negative eigenvalue(s) less than -1.0e-06"
==== 1 failed, 13459 passed, 5327 skipped, 24 xfailed in 939.73s (0:15:39) =====

Here is the output error message:

FIM provided is not positive definite. It has one or more negative eigenvalue(s) less than -1.0e-06

Here is the expected error message:

FIM provided is not positive definite. It has one or more negative eigenvalue(s) less than -1.0e-06

I compared these side-by-side in a text editor, and they look identical to me. I did not try to text difference them.

mrmundt · 2025-03-26T13:45:16Z

@adowling2 - I would guess this is some odd Python formatting problem (e.g., it might not be a direct str-to-str comparison like it appears). If you try to change the regex to something like:

       with self.assertRaisesRegex(
            ValueError,
            "FIM provided is not positive definite. It has one or more negative eigenvalue(s) .*")

Does it still fail? If so, then there is something wonky with the way the specific value is being formatted.

jsiirola · 2025-03-26T14:24:03Z

@adowling2: assertRaisesRegex is testing the message against the provided regular expression, and not doing a simple string comparison. ( and ) are grouping operators in regexes -- if you want to match those characters, they have to be escaped \( and \) in the regex. (Also note that as soon as you are putting backslashes in regex strings, you probably want to declare them as raw strings (r"...") to avoid "escaping hell").

adowling2 · 2025-03-26T20:23:19Z

I should have realized that 😥 Thank you!

adowling2 · 2025-03-27T00:10:19Z

I'm fairly new to pytest. Here is some of the output for the failed 3.12 cases:

==================================== ERRORS ====================================
________ ERROR at teardown of TestSolvers.test_fixed_binaries_0_gurobi _________

self = <contextlib._GeneratorContextManager object at 0x7fb0ed718140>
typ = None, value = None, traceback = None

    def __exit__(self, typ, value, traceback):
        if typ is None:
            try:
>               next(self.gen)
E               OSError: [Errno 29] Illegal seek

/opt/hostedtoolcache/Python/3.12.9/x64/lib/python3.12/contextlib.py:144: OSError
______ ERROR at setup of TestSolvers.test_fixed_binaries_1_gurobi_direct _______

self = <contextlib._GeneratorContextManager object at 0x7fb107fbdbe0>
typ = None, value = None, traceback = None

    def __exit__(self, typ, value, traceback):
        if typ is None:
            try:
>               next(self.gen)
E               OSError: [Errno 29] Illegal seek

/opt/hostedtoolcache/Python/3.12.9/x64/lib/python3.12/contextlib.py:144: OSError
_____ ERROR at teardown of TestSolvers.test_fixed_binaries_1_gurobi_direct _____

adowling2 · 2025-03-27T00:12:13Z

Here is the output for the failed Windows/3.12 test:

================================== FAILURES ===================================
___________________ TestSolvers.test_fixed_binaries_4_highs ___________________

a = (<pyomo.contrib.solver.tests.solvers.test_solvers.TestSolvers testMethod=test_fixed_binaries_4_highs>,)
kw = {}

    @wraps(func)
    def standalone_func(*a, **kw):
>       return func(*(a + p.args), **p.kwargs, **kw)

C:\Miniconda\envs\test\Lib\site-packages\parameterized\parameterized.py:620: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
pyomo\contrib\solver\tests\solvers\test_solvers.py:1342: in test_fixed_binaries
    res = opt.solve(m)
pyomo\contrib\solver\common\persistent.py:529: in solve
    res = self._solve()
pyomo\contrib\solver\solvers\highs.py:175: in _solve
    with capture_output(output=TeeStream(*ostreams), capture_fd=True):
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <pyomo.common.tee.capture_output object at 0x0000015E7AE8FDA0>, et = None
ev = None, tb = None

    def __exit__(self, et, ev, tb):
        # Restore any file descriptors we comandeered
        if self.fd_redirect is not None:
            for fdr in reversed(self.fd_redirect):
                fdr.__exit__(et, ev, tb)
            self.fd_redirect = None
        # Check and restore sys.stderr / sys.stdout
        FAIL = self.tee.STDOUT is not sys.stdout
        self.tee.__exit__(et, ev, tb)
        if self.output_stream is not self.output:
            self.output_stream.close()
        sys.stdout, sys.stderr = self.old
        self.old = None
        self.tee = None
        self.output_stream = None
        # Clean up our temporary override of the local logger
        self.temp_log_handler.flush()
        logger.removeHandler(self.temp_log_handler)
        if self.capture_fd:
            self.temp_log_stream.flush()
            self.temp_log_stream.close()
        logger.propagate = self._propagate
        self.temp_log_stream = None
        self.temp_log_handler = None
        if FAIL:
>           raise RuntimeError('Captured output does not match sys.stdout.')
E           RuntimeError: Captured output does not match sys.stdout.

pyomo\common\tee.py:307: RuntimeError
------------- generated xml file: D:\a\pyomo\pyomo\TEST-pyomo.xml -------------
=========================== short test summary info ===========================
FAILED pyomo/contrib/solver/tests/solvers/test_solvers.py::TestSolvers::test_fixed_binaries_4_highs - RuntimeError: Captured output does not match sys.stdout.
=== 1 failed, 12827 passed, 5990 skipped, 24 xfailed in 1368.44s (0:22:48) ====
Error: Process completed with exit code 1.

jsiirola · 2025-03-27T04:45:52Z

@adowling2, the RuntimeError from tee.py is unrelated to your PR, and should be resolved once we merge #3537. I haven't seen the _GeneratorContextManager exceptions before, but it is plausible that it is also related to the capture_output bugs that #3537 resolves.

Corrected initialization when using only lower diagonal of FIM

69091fa

Ran black

6e1351e

mrmundt reviewed Mar 24, 2025

View reviewed changes

blnicho approved these changes Mar 24, 2025

View reviewed changes

blnicho added the AT: PRE-TEST INSPECTED label Mar 24, 2025

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Mar 24, 2025

Began adding a test with a negative definite prior... this raises the…

1d0728b

… question that we should probably have some extra error checking

Added error messages, updated tests and ran black.

cda4545

Still need to check the tests work

adowling2 added 4 commits March 25, 2025 14:17

Updated one test.

2a2455b

Updated tests file

809aaea

Ran black

eb74ecb

Debugged all but one test

5ddfc06

adowling2 mentioned this pull request Mar 25, 2025

Pyomo.DoE: adding more verbose output for sensitivity analysis #3525

Merged

adowling2 added 3 commits March 25, 2025 18:21

Merge branch 'main' into pyomo-doe-fix-initialization

232386b

Ran black

e004df8

Merge branch 'pyomo-doe-fix-initialization' of https://github.com/ado…

9966cff

…wling2/pyomo into pyomo-doe-fix-initialization

adowling2 added 2 commits March 26, 2025 16:17

Simplified error message checking to get around weird string formatting

1e38641

Ran black

d662444

Tests pass now

383883f

Removed commented out assertion checks

c28c249

Merge branch 'main' into pyomo-doe-fix-initialization

0fa9c2b

blnicho approved these changes Apr 2, 2025

View reviewed changes

blnicho added the AT: PRE-TEST INSPECTED label Apr 2, 2025

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Apr 2, 2025

blnicho requested a review from mrmundt April 3, 2025 17:34

mrmundt approved these changes Apr 9, 2025

View reviewed changes

blnicho added this to Pyomo Release April 2025 Apr 14, 2025

github-project-automation bot moved this to Todo in Pyomo Release April 2025 Apr 14, 2025

blnicho moved this from Todo to Review In Progress in Pyomo Release April 2025 Apr 14, 2025

blnicho merged commit ae863b6 into Pyomo:main Apr 15, 2025
60 of 65 checks passed

github-project-automation bot moved this from Review In Progress to Done in Pyomo Release April 2025 Apr 15, 2025

Conversation

adowling2 commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes # .

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

Uh oh!

adowling2 commented Mar 22, 2025

Uh oh!

adowling2 commented Mar 23, 2025

Uh oh!

mrmundt left a comment

Choose a reason for hiding this comment

Uh oh!

blnicho left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

adowling2 commented Mar 24, 2025

Uh oh!

adowling2 commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adowling2 commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adowling2 commented Mar 25, 2025

Uh oh!

adowling2 commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrmundt commented Mar 26, 2025

Uh oh!

jsiirola commented Mar 26, 2025

Uh oh!

adowling2 commented Mar 26, 2025

Uh oh!

adowling2 commented Mar 27, 2025

Uh oh!

adowling2 commented Mar 27, 2025

Uh oh!

jsiirola commented Mar 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adowling2 commented Mar 22, 2025 •

edited

Loading

codecov bot commented Mar 24, 2025 •

edited

Loading

adowling2 commented Mar 24, 2025 •

edited

Loading

adowling2 commented Mar 24, 2025 •

edited

Loading

adowling2 commented Mar 26, 2025 •

edited

Loading