Draft: Improve handling of covariance calculation

[eendebakpt]

Description

In this PR we make two changes to the calculation of the covariance

• If one of the diagonal elements of the calculated covariance is negative we know the result is not good, and we set the covariance to None. The None value was already used when no covariance calculation was made, or for other issues with the calculation, so this should have limited impact on users.
• In case of a LinAlgError or ValueError in the calculation a None is returned for the covariance (as before) and the original parameter values are restored.

A discussion on the changes is in #812. Because of the first change, the warnings generated when the square root of the negative diagonal elements of the covariance matrix are taken have been removed. If this behaviour is not acceptable, I suggest adding a dedicated warning, e.g.

     if cov_x.diagonal().min() < 0:
            # we know the calculated covariance is incorrect, so we set the covariance to None
            warnings.warn('calculated covariance has negative elements, setting to None', CovarianceWarning)
            cov_x = None

Type of Changes

• Bug fix
• New feature
• Refactoring / maintenance
• Documentation / examples

Tested on

Python: 3.10.8 (tags/v3.10.8:aaaf517, Oct 11 2022, 16:50:30) [MSC v.1933 64 bit (AMD64)]

lmfit: 1.0.3, scipy: 1.9.2, numpy: 1.23.4, asteval: 0.9.27, uncertainties: 3.1.7

Verification

Have you

• included docstrings that follow PEP 257?

• referenced existing Issue and/or provided relevant link to mailing list?

• verified that existing tests pass locally?

• verified that the documentation builds locally? Documentation does not build (error: FileNotFoundError: test_splinepeak.dat not found.)

• squashed/minimized your commits and written descriptive commit messages?

• added or updated existing tests to cover the changes?
• updated the documentation and/or added an entry to the release notes (doc/whatsnew.rst)?
• added an example?

[newville]

@eendebakpt Thanks -- I think this looks fine. If there are any objections or concerns, I would intend to merge this early next week.

[reneeotten]

@eendebakpt @newville I am fine with the outcome of the suggested change, just not sure about the way it's coded. Admittedly, I have looked only shortly so perhaps I am missing something but:

• why do we reconstruct cov_x in the case of of an LinAlgError or ValueError and then eventually anyway return None - that doesn't make sense to me

Is there a reason that the added block:

        if cov_x.diagonal().min() < 0:
            # we know the calculated covariance is incorrect, so we set the covariance to None
            cov_x = None

cannot be part of the try clause and then we can just leave the return None in the except part?

[newville]

@reneeotten That was my suggestion, replacing an immediate return None in the expect block.

Basically, I thought it would be better to run through the entire method, avoiding an early exit, so that the best-fit values were always restored. It didn't seem like it all fits in a single exception block anymore now that @eendebakpt adds an explicit check for negative (impossible!) diagonal values. Deliberately setting up cov_x to fail the "valid diagonal values" test seemed ok to me. but I'm sure there are other ways to accomplish this.

Would a comment in the code be acceptable?

[eendebakpt]

@reneeotten Moving the check inside the try block indeed seems better, I updated the code. In the except part I set cov_x to None, so the parameters get restored.

[reneeotten]

@reneeotten Moving the check inside the try block indeed seems better, I updated the code. In the except part I set cov_x to None, so the parameters get restored.

Thanks @eendebakpt - that is indeed what I was thinking about! The "keep return None" in my initial comment was not correct, sorry about that setting cov_x = None is indeed the correct thing to do.

[newville]

@eendebakpt @reneeotten that's okay with me. Thanks!