test on cholesky factor instead of covariance

[lostella]

Issue #, if available: addresses #41, doesn't necessarily solve it

Description of changes: this PR turns the test on covariance vs empirical covariance matrices, into a test on their Cholesky factors. I don't have a formal justification for this, but note that

1. squaring stuff can amplify errors
2. we test for the standard deviation in the Gaussian case (and not for the variance)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[szha]

Job PR-42/1 is complete.
Docs are uploaded to http://gluon-ts-staging.s3-accelerate.dualstack.amazonaws.com/PR-42/1/index.html

[mbohlkeschneider]

I would move this into a separate test with clear naming such as test_cholesky and test_mean so it is clear from the test names what we do and what we don't check.

Otherwise this can be missed easily. Maybe add a todo that we currently don't test for variance?

[lostella]

Splitting this in two separate tests looks a bit too much to me. Note that this is how we structure all other sampling tests (also for scalar distributions, see test_sampling above), and that checking the Cholesky factor of the covariance matrix reduces to checking the standard deviation in the scalar case. We do test for the covariance, only indirectly.

[mbohlkeschneider]

My major gripe with the change is that the actual variance is not tested anymore which means that any bug that is hidden from getting from the cholesky factor to the variance in the distribution will be missed. So it is not a proper test to test the distribution.

I would rather bump up the tolerance and add a todo. For the release, I think it would be better to remove the distribution if it does not work properly. Do we have any models that depend on that?

[lostella]

I think here we should not use Distribution.mean() and Distribution.variance() at all, since this is a test for Distribution.sample().

As argument in favor of this: you could construct a Gaussian distribution that just multiplies by two the mean and stddev you constructed it with. If you then test the samples it produces against distr.mean and distr.variance, all will be fine. But nothing is fine in reality.

[benidis]

After the "splines" incident my opinion is to check for everything. Having said that, the variance is computed based on an mxnet function:

 def variance(self) -> Tensor:
        return self.F.linalg_gemm2(self.L, self.L, transpose_b=True)

This means that there is no room for error from our side there right? Maybe we can add one more test for this case eitherway (which makes sense if we assume that cholesky is more stable):

assert np.allclose(
        np.linalg.cholesky(np.cov(np_samples.transpose())),
        np.linalg.cholesky(distr.variance.asnumpy()),
        atol=0.1,
        rtol=0.1,
    )

[lostella]

Or use the third side of the triangle:

assert np.allclose(
        params['L'].asnumpy(),
        np.linalg.cholesky(distr.variance.asnumpy()),
    )

[benidis]

Yes, maybe this is even better if we just want to check if the variance is computed correctly since there are no samples involved.

[lostella]

I can add that, but again I think that would fit another test, where methods other than .sample are tested. I will open a separate issue about refactoring these tests.

[lostella]

Which goes in the direction of what @mbohlkeschneider was initially proposing :-)

[mbohlkeschneider]

Then let's add the issue and do this with a later PR. Thank you for bringing this up!

[mbohlkeschneider]

Thank you!

[szha]

Job PR-42/2 is complete.
Docs are uploaded to http://gluon-ts-staging.s3-accelerate.dualstack.amazonaws.com/PR-42/2/index.html

[szha]

Job PR-42/3 is complete.
Docs are uploaded to http://gluon-ts-staging.s3-accelerate.dualstack.amazonaws.com/PR-42/3/index.html