Improve specializations of Hybrid::size()

The interfaces tested subsequently for Hybrid::size(const T& t) are now

• standard tuple-like interface std::tuple_size<T>::value (e.g. needed for std::tuple),
• static constexpr T::size() (e.g. needed for Dune::TupleVector),
• static constexpr T::N() (e.g. needed for Dune::MultiTypeBlockMatrix),
• dynamic member function t.N() (e.g. needed for Dune::Matrix),
• dynamic member function t.size() (e.g. needed for std::vector).

Unfortunately the matrix interface is rather inconsistent. While Dune::DenseMatrix provides a dyanmic size() method, the dynamic dense matrix Dune::Matrix does not. The only consistent interface to get the number of rows is the N() method which exists in dynamic (e.g. Dune::Matrix) and constexpr static (e.g. Dune::MultiTypeBlockMatrix) flavour. Hence to support matrices consistently, we must add support for those cases, too.

For the static constexpr N(), this was made neccessary, because of the removal of static consexpr Dune::MultiTypeBlockMatrix::size(). The dynamic case should be added for consistency to also support Dune::Matrix and Dune::BCRSMatrix. As a by-product, this allows to get rid of the FieldMatrix specialization.

Support for the N() cases was removed from the MR and left to future discussion. This MR now removes an obsolete special case for FieldVector, extends the test to cover this case, and fixes the documentation.

For me the question is: What should size() of a matrix represent? In most libraries, the size is the number of entries in a matrix and thus not the number of rows. Hence, the Hybrid::size() would introduce an (external) matrix interface that would not be consistent with the (internal) matrix interface. I think the matrix.size() method was removed because of this. A specialization for .N() or ::N() is, in my opinion, not the right way to go.

Maybe better: introduce a Hybrid::rows() and Hybrid::cols() function. It is somewhere already proposed in a MR (I have to look this up).

While I personally would not use a size function for matrices, albeit its proper meaning (see discussion before), I don't want to block this addition if it helps someone. Independent of this, a similar hybrid interface for number of rows/columns might be useful and could be introduce separately.

mentioned in commit fufem/dune-matrix-vector@9702bdbf

mentioned in merge request fufem/dune-matrix-vector!13 (merged)

resolved all threads

While you (@carsten.graeser) mentioned that the other MR in dune-istl solved your specific problem, how should we proceed with this MR? We have discussed pro and cons for size() function for matrices and there are still different opinions about this. It is just an interface addition, so does not change the working old code, right? It does not prevent from implementing other utilities like Hybrid::numRows() separately. We do not have a clear picture of a nice and consistent matrix interface and it will not come soon (issues and MRs about this topic are open but need deeper discussions than possible here on GitLab).

So, I'm fine with merging this if it still helps. Otherwise it should be either marked as Draft to have it as a discussion archived, or closed and reopened if this interface comes up again.

Since there's no consensus on the specializations for N() I'll remove this from the present MR. For dune-fufem I now implemented a separate hybridRowIndexAccess() that clones Dune::Functions::hybridIndexAccess() with size() replaced by N().

However, the question may come up again in the context of dune-istl#105.

I now removed the matrix cases and only kept the further cleanup and documentation fix.

Thanks. I think this can be merged.

added 45 commits

• 5b636ee9...daebfb62 - 44 commits from branch master
• 43aaa9d0 - Improve specializations of Hybrid::size()

Compare with previous version

changed the description

assigned to @simon.praetorius

approved this merge request

added 47 commits

• 43aaa9d0...364d0fd4 - 46 commits from branch master
• 0be82d48 - Improve specializations of Hybrid::size()

Compare with previous version

enabled an automatic merge when the pipeline for 0be82d48 succeeds

mentioned in commit a2c094ff

merged