fixed fail 2 issue number 181

[Karl-JT]

Hi, I'm trying to contribute by addressing issue #181 fail 2 by identifying sparse diagonal matrices.

[jakobsj]

Many thanks @Karl-JT for submitting this! We'll take a look and get back to you asap.

[nabriis]

Thank you @Karl-JT for the contribution! This seems to indeed have fixed the bug with sparse diagonal sqrtprec.

If you could be so kind as to also add a test in tests/test_distribution.py that checks against such future errors occurring. The code from the test would be something like this:

def test_Gaussian_from_sparse_sqrtprec():
    """ Test Gaussian distribution from sparse sqrtprec is equal to dense sqrtprec """
    N = 10; M = 5

    sqrtprec = sp.sparse.spdiags(np.random.randn(N), 0, N, N)

    y_from_sparse = cuqi.distribution.Gaussian(mean = np.zeros(N), sqrtprec = sqrtprec)
    y_from_dense = cuqi.distribution.Gaussian(mean = np.zeros(N), sqrtprec = sqrtprec.todense())

    assert y_from_dense.logpdf(np.ones(N)) == y_from_sparse.logpdf(np.ones(N))

For how to run tests please see https://cuqi-dtu.github.io/CUQIpy/dev/how_to.html

Option to fix entire issue

As to the issue in #181, I believe it would be good to check if the sqrtprec was not square and raise a ValueError in that case. If you are up for implementing that also it would be awesome :)

To follow "test driven development" you can add this test first and then work on raising the error such that the test passes.

def test_Gaussian_sqrtprec_must_be_square():
    """ Test Gaussian distribution raises ValueError if sqrtprec (sparse and dense) is not square """
    N = 10; M = 5

    # Create non square sqrtprec as sparse (later converted to dense)
    sqrtprec = sp.sparse.csr_matrix(np.random.randn(N, M))
    

    with pytest.raises(ValueError, match="sqrtprec must be square"):
        cuqi.distribution.Gaussian(mean = np.zeros(N), sqrtprec = sqrtprec)

    with pytest.raises(ValueError, match="sqrtprec must be square"):
        cuqi.distribution.Gaussian(mean = np.zeros(N), sqrtprec = sqrtprec.todense())

[Karl-JT]

Thank you very much @nabriis for the guide. I have updated it with an additional check on whether sqrtprec is square. The two suggested tests are included as well.

[nabriis]

@Karl-JT Thank you for the quick action. The changes look good to me. I will hand it to @jakobsj for the final review.

[jakobsj]

Excellent, I have no further comments. Thank you very much for this contribution, we're very pleased to receive and merge this in.

Just in case you are interested to contribute further, feel free to take a look at our list of open issues
https://github.com/CUQI-DTU/CUQIpy/issues
or submit a new issue in case you discover a bug, have a suggestion for improvement of code or documentation or would like to propose a new feature.

Thanks again,
Jakob

[nabriis]

Thanks again. A short note. The changes will automatically be part of our next release, but can already be acquired via our pre releases installed with:

pip install cuqipy --pre