One more bugfix

[davschneller]

Fix e.g. A - a * B.

(it's decomposed into -1.0 * (a * B) which in turn was not allowed by the __neg__ operator—but the __mul__ operator did allow it)

[vikaskurapati]

Why don't we need to check multiple scalar units here?

[davschneller]

That's done via __rmul__ calling __mul__; there we auto-merge with existing scalar multiplications. If we don't have a scalar multiplication, then the old and the new version behave identically (i.e. __mul__ creates a ScalarMultiplication node).
EDIT: that didn't happen so far; nevermind. Both should be completely identical.

The previous implementation (i.e. self._checkMultipleScalarMults()) just aborted at that point instead.

(that said—the case (a * A) * (b * B) should, by tracing the code paths, also still fail with the new version, instead of turning it into (a * b) * A * B or the likes ... Though that might also be rather straight-forward to fix I think)

[vikaskurapati]

Seems good to me from what I understand. Could some small test be added for this?

Did you check SeisSol along with this version of YaTeTo to check that there were no issues?

[davschneller]

That issue didn't occur in SeisSol so far iirc (or only the non-changed code path); I fixed this issue due to an external comment (from tandem).

But yeah; I've added a Yateto self test for that issue. (which does, albeit, not yet check if the result also conforms with what we'd really "expect"; i.e. with pre-computed test data—that'll probably be more work to set up properly)

[davschneller]

Ok; nevermind all that. Turns out the whole issue will still need a tiny bit more work—probably more like a pass that'll merge scalar multiplications and/or ad-hoc multiply them before an operation (it's doable; but it'll take me a tiny bit more time—I'm setting everything back to draft for now).

[vikaskurapati]

That issue didn't occur in SeisSol so far iirc (or only the non-changed code path); I fixed this issue due to an external comment (from tandem).

This may be an unrelated comment, but do the current tests with YaTeTo have tests that are generic enough to ensure functionalities across different software packages? And do you think it is a good idea to invite developers from Tandem, e.g., Piyush, to check such PRs which were intended for them. This may help with testing respective features.

But yeah; I've added a Yateto self test for that issue. (which does, albeit, not yet check if the result also conforms with what we'd really "expect"; i.e. with pre-computed test data—that'll probably be more work to set up properly)

I don't fully understand this comment, tbh, but that's probably due to my limited knowledge of the entire software package. But what is the self-test check that is currently implemented? And if we could actually check with something we know is correct as a test, it would be great, imo.

[davschneller]

That issue didn't occur in SeisSol so far iirc (or only the non-changed code path); I fixed this issue due to an external comment (from tandem).

This may be an unrelated comment, but do the current tests with YaTeTo have tests that are generic enough to ensure functionalities across different software packages? And do you think it is a good idea to invite developers from Tandem, e.g., Piyush, to check such PRs which were intended for them. This may help with testing respective features.

Yeah, we can do that imo. Reagarding the tests: we do check different CPU codegen packages right now; but not so much GPU codegen packages—mostly due to the instability of the CI and hardware access. (probably, CUDA/AdaptiveCpp would be possible to test against)
"Dry" testing for those, i.e. to just compile but not run them, would be easily possible.

But yeah; I've added a Yateto self test for that issue. (which does, albeit, not yet check if the result also conforms with what we'd really "expect"; i.e. with pre-computed test data—that'll probably be more work to set up properly)

I don't fully understand this comment, tbh, but that's probably due to my limited knowledge of the entire software package. But what is the self-test check that is currently implemented? And if we could actually check with something we know is correct as a test, it would be great, imo.

The self-test is currently implemented IIRC as follows: take some pseudo-random initialization data, and compute the reference solution from the un-optimized kernels (i.e. no matrix multiplication/tensor contraction optimization; everything might as well be one big loop).
That works well, as long as the un-optimized kernels work as they should—or in other words: Yateto tests against its own optimizations that it performs on the kernel—based on its own understanding what the kernel is supposed to do. Which is not necessarily the user's intended meaning—e.g. problems during the AST building (or that happen during the unit test construction) itself are not detected by the tests therefore.

Testing that is possible, if you were to manually implement some kernels yourselves and compare against the kernel you'd get. Not 100%ly sure how to best do it idiomatically; it can range from just having a C++ file with some kernels and the sample solutions coded in there, to providing extra test cases for Yateto (e.g. just provide data for all tensors you'd expect as input and output—but you're yourself responsible for supplying the correct input data).

[vikaskurapati]

Yeah, we can do that imo. Reagarding the tests: we do check different CPU codegen packages right now; but not so much GPU codegen packages—mostly due to the instability of the CI and hardware access. (probably, CUDA/AdaptiveCpp would be possible to test against) "Dry" testing for those, i.e. to just compile but not run them, would be easily possible.

Okay. Any checks possible would be nice to have, for sure. If you are worried about bloating the test suite too much, we could then choose the more important ones and turn off the non-important ones later on.

The self-test is currently implemented IIRC as follows: take some pseudo-random initialization data, and compute the reference solution from the un-optimized kernels (i.e. no matrix multiplication/tensor contraction optimization; everything might as well be one big loop). That works well, as long as the un-optimized kernels work as they should—or in other words: Yateto tests against its own optimizations that it performs on the kernel—based on its own understanding what the kernel is supposed to do. Which is not necessarily the user's intended meaning—e.g. problems during the AST building (or that happen during the unit test construction) itself are not detected by the tests therefore.

Okay. I am just thinking out loud here: Could we, in that case, implement some basic checks as tests, for example, multiplying a random matrix by an identity should always return the same matrix, etc.? Or something similar, which are simple to implement but we know that they should be this way?

Testing that is possible, if you were to manually implement some kernels yourselves and compare against the kernel you'd get. Not 100%ly sure how to best do it idiomatically; it can range from just having a C++ file with some kernels and the sample solutions coded in there, to providing extra test cases for Yateto (e.g. just provide data for all tensors you'd expect as input and output—but you're yourself responsible for supplying the correct input data).

This sounds okay to me, imo. Maybe we could take a few matrices from SeisSol, and check against what we expect in those scenarios? We could have a more extensive discussion about it later if you want.

[vikaskurapati]

Yeah, we can do that imo.

Just for the sake of information, I added @piyushkarki with write access.

[davschneller]

• I think the test suite is still good w.r.t. size; it's actually pretty small. (and also, e.g., I once tried to deactivate some optimizations; and it caused an error—possibly one that was causing issues before Fix two optimizer-related bugs #94 shadowed them again).
• Yeah, that sounds really possible. I.e. just set a tensor to the Kronecker delta (though the sparsity patterns might play us tricks here). We'll still get different results if we have encoded constants set to not 1, or due to sums. But yes—that sounds like a really good idea IMO. The only thing we'll need to keep in mind probably is that the compile time for the tests right now is already pretty long. So we'd need to somehow automatically split up the test files in the best case; and compile each of them individually. (that'll be an even larger issue for Meta Kernel Generation #96 )
• Yup, that sounds good. We could e.g. re-create the computation in numpy and compare results.

(by the way—I've run the current Yateto-internal tests for SeisSol for all kernels (i.e. all orders, equations, precisions, etc.) on the CPU in SeisSol/#1421—they basically all passed save for about six with some slightly too big relative errors in some extreme cases—which might also come from the unit test implementation)