Fix assembly of Real matrices

[connorjward]

Description

Fixes #3837 and (I claim) tidies up the logic in assemble.py slightly.

The core of the issue was that we were not unpacking the Dat or Global from the MATPYTHON matrices that we use for the Real space.

I need some help coming up with a good test for it.

[github-actions]

	Tests	Passed ✅	Skipped ⏭️	Failed ❌
Firedrake complex	8091 ran	6507 passed	1584 skipped	0 failed

[github-actions]

	Tests	Passed ✅	Skipped ⏭️	Failed ❌
Firedrake real	8097 ran	7311 passed	786 skipped	0 failed

[JHopeCollins]

Test ideas:

1. Can you explicitly assemble the matrix (rather than relying on the solver to do it) and check that all the entries are correct?
2. Is it worth adding tests with other function space layouts too, e.g. CG*R*CG or R*CG*R?

[connorjward]

Test ideas:

1. Can you explicitly assemble the matrix (rather than relying on the solver to do it) and check that all the entries are correct?
2. Is it worth adding tests with other function space layouts too, e.g. CG*R*CG or R*CG*R?

I think the issue is less to do with the nonlinear solve and more that passing tensor=existing_matrix to assemble does not work. So thanks for the suggestion I will try to do something like that.

[JHopeCollins]

Why do you want to use the tensor argument? Why not just ask assemble to create the matrix for you?

[connorjward]

Why do you want to use the tensor argument? Why not just ask assemble to create the matrix for you?

This is for repeated assembly so it's more efficient.

[JHopeCollins]

Why do you want to use the tensor argument? Why not just ask assemble to create the matrix for you?

This is for repeated assembly so it's more efficient.

I meant just for this test, but that makes sense.
Do we currently support passing a matrix tensor for matrices without a Real space? If not, then I don't think it's needed for this PR - especially seeing as this is fixing a bug that users are currently running into.

[connorjward]

Why do you want to use the tensor argument? Why not just ask assemble to create the matrix for you?

This is for repeated assembly so it's more efficient.

I meant just for this test, but that makes sense. Do we currently support passing a matrix tensor for matrices without a Real space? If not, then I don't think it's needed for this PR - especially seeing as this is fixing a bug that users are currently running into.

Yes you can always pass tensor (apart from for zero forms). I have come up with a much more minimal test that specifically tests the thing I fixed.

[JHopeCollins]

I have come up with a much more minimal test that specifically tests the thing I fixed.

That's a useful test, but how come you also removed the original MFE? Are we guaranteed that if the new test passes, then that MFE will pass? I'm not convinced that it is guaranteed, because the new test doesn't explicitly specify mat_type = "nest". Shouldn't we have both tests?

[connorjward]

That's a useful test, but how come you also removed the original MFE? Are we guaranteed that if the new test passes, then that MFE will pass? I'm not convinced that it is guaranteed, because the new test doesn't explicitly specify mat_type = "nest". Shouldn't we have both tests?

I don't really know. In general I want to avoid the proliferation of "big" tests that slow the test suite down. Also we implicitly specify mat_type=nest when assemble a matrix with Real blocks.

[JHopeCollins]

In general I want to avoid the proliferation of "big" tests that slow the test suite down.

Why do you think it's big? Unless I'm reading the logs correctly it passed in about 2 seconds.

I don't really know. ... Also we implicitly specify mat_type=nest when assemble a matrix with Real blocks.

The original bug was for mat_type="nest" so we should be sure that the test will cover this case. Relying on implicit behaviour that could change in the future doesn't achieve this.

[connorjward]

In general I want to avoid the proliferation of "big" tests that slow the test suite down.

Why do you think it's big? Unless I'm reading the logs correctly it passed in about 2 seconds.

Maybe not "big" but "non-specific"? I don't necessarily think it's reasonable for a package to fill its test suite with snippets of user code when I can add a unit test that directly validates the change that I made. I'd be happy to discuss this in person at some point maybe.

I don't really know. ... Also we implicitly specify mat_type=nest when assemble a matrix with Real blocks.

The original bug was for mat_type="nest" so we should be sure that the test will cover this case. Relying on implicit behaviour that could change in the future doesn't achieve this.

Sure. I will make the test more explicit.

[JHopeCollins]

Maybe not "big" but "non-specific"? I don't necessarily think it's reasonable for a package to fill its test suite with snippets of user code when I can add a unit test that directly validates the change that I made.

Ok this makes more sense (although I would word it: "that directly validates that the bug is fixed", but we can be pedantic about it some other time). This is why I was asking if the original MFE would definitely pass if the new test passed. If you're happy that that is the case now mat_type="nest" is specified explicitly then yes, the original MFE is now obsolete as a test.

I'd be happy to discuss this in person at some point maybe.

Sure, happy to chat when we're both in the office.

[connorjward]

@ksagiyam can you take another look?