Pyomo.DoE: Corrected initialization when using only lower diagonal of FIM

[adowling2]

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:

1. I agree my contributions are submitted under the BSD license.
2. 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]

@djlaky Check this out

[adowling2]

@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]

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

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]

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

I'll work on a test

[adowling2]

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

[adowling2]

TODO:

• Check that new tests runs correctly

[adowling2]

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.

[adowling2]

@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]

@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]

@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]

I should have realized that 😥 Thank you!

[adowling2]

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]

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]

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