Fix test for mixed-ness in Function.sub, and fix Cofunction.sub to match

[JHopeCollins]

Fixes #3933

1. Fixes the test for mixed-ness to check the UFL element rather than the length - this gets the correct answer for "mixed" spaces with only one component.
2. Updates Cofunction.sub in line with Function.sub.

[github-actions]

	Tests	Passed ✅	Skipped ⏭️	Failed ❌
Firedrake real	8162 ran	7487 passed	675 skipped	0 failed

[github-actions]

	Tests	Passed ✅	Skipped ⏭️	Failed ❌
Firedrake complex	8156 ran	6682 passed	1474 skipped	0 failed

[JHopeCollins]

@connorjward @ksagiyam I removed the bounds checking in sub for FunctionSpace, Function, and Cofunction because this check will be done by the subfunctions or _components tuple anyway. However, this is causing the tests to fail because we expect that FunctionSpace.sub will fail if passed a negative index. Given that negative indices are a standard python feature, and here they are doing the usual python thing, is there any reason to keep this restriction?

[connorjward]

LGTM more or less

[connorjward]

@connorjward @ksagiyam I removed the bounds checking in sub for FunctionSpace, Function, and Cofunction because this check will be done by the subfunctions or _components tuple anyway. However, this is causing the tests to fail because we expect that FunctionSpace.sub will fail if passed a negative index. Given that negative indices are a standard python feature, and here they are doing the usual python thing, is there any reason to keep this restriction?

I'm happy to permit negative indices and do the Pythonic thing. There is no way that this will break user code to change.

[JHopeCollins]

I'm happy to permit negative indices and do the Pythonic thing. There is no way that this will break user code to change.

Great, I've updated the test so that it checks a negative index that is actually out of bounds.

[ksagiyam]

I think those bound checks are there to give clearer error message:

"Invalid component {i}, not in [0, {bound})"

vs.

IndexError: list index out of range

so I think we should keep them.

[JHopeCollins]

I think those bound checks are there to give clearer error message:

"Invalid component {i}, not in [0, {bound})"

vs.

IndexError: list index out of range

so I think we should keep them.

That's the usual python message for bad indices so personally I'm fine with it. If we really want a slightly more detailed one then we should be catching the IndexError and then adding to the message. But if we think it's really that important then shouldn't we also add this message to subfunctions for FunctionSpace, DualSpace, MixedFunctionSpace, Function, Cofunction, and also anywhere else where we expose a tuple to the user?
It just seems quite heavy handed for the problem it's addressing.

[connorjward]

I think those bound checks are there to give clearer error message:

"Invalid component {i}, not in [0, {bound})"

vs.

IndexError: list index out of range

so I think we should keep them.

That's the usual python message for bad indices so personally I'm fine with it. If we really want a slightly more detailed one then we should be catching the IndexError and then adding to the message. But if we think it's really that important then shouldn't we also add this message to subfunctions for FunctionSpace, DualSpace, MixedFunctionSpace, Function, Cofunction, and also anywhere else where we expose a tuple to the user? It just seems quite heavy handed for the problem it's addressing.

I agree. If we really cared about this we should probably have something like

try:
  return data[i]
except IndexError as e:
  raise FunctionSpaceIndexOutOfBoundsException("...") from e

but I don't think it's terribly important.

---

Providing an informative error message is fine. Adding our own bounds checking logic when Python already does the right thing is not.

[ksagiyam]

I see. I am fine with that.