DenseMatrix' bounds check to restrictive

The DenseMatrix bounds check are too restrictive with respect of their type. For example dune-common/dune/common/densematrix.hh:393 tests for x.N() which is not provided by std::vector. Thus I cannot compile dynmatrixtest with DUNE_FMatrix_WITH_CHECKING defined.

This is already addressed in !10 (merged), no?

In some sense... the checks pass now, but this is due to disabled tests for some class I personally consider not fulfilling the required interface.

The question is mainly how we should proceed... see my mail to dune-devel.

What shall we do? Replace x.N() by x.size()? This is what I would think is the best solution, or shall we aim at a more general solution/interface, where use an ADL overload with a free function size?

It's just for a check. What about wrapping the test in a TMP'ed if which a second case for std::vector?

No, it is not just for the check. The check relies upon a particular interface and the question is what is the expected interface for vectors you can pass to the matrix. If we are strict, these would be things following the DenseVector interface, which is definitely not fulfilled by std::vector.

An other place where we require a richer interface is the IdentityMatrix class, which forwards the usmv to the axpy of the vector.

I missed that. Then I would drop support of std::vector. Or is there a valid use case?

I vote for replacing x.N() by x.size(). I stumbled over the method 'N' in BlockVector the other day, and until reading this thread here I actually thought that having BlockVector::N was a bug. What is the reason for having it?

I'm also in favor of some type of size, either x.size() or size(x). I just think that std::vector is the wrong argument here.

Even though the DenseMatrix class lives on blocklevel zero, so that this is not an issue in practice, it might be worth noting that of the four vector classes that one can do arithmetic with (did I miss one?), namely DenseVector (DynamicMatrix , FieldMatrix), BlockVector, VariableBlockVector, and MultiTypeBlockVector, each one has a size() but MultiTypeBlockVector does not have N().

That said, there isn't really an issue here anymore, is there? Everything that's supposed to work, does work. Interface changes could also be discussed in a more general context in dune-istl#7 (which is about matrices, but you can't really talk about matrices without also talking about vectors).

I guess it depends on what kind of vectors you are talking about. One might imagine sparse vectors, like compressed_base_array_unmanaged< B, A >, here N() and size() would be different.

@markus.blatt: Right. For a sparse vector, N() would be what we need (namely the size of the mathematical object) and size() would only return the number of entries that are actually used (an implementation detail, really). So we should stick with N() then, no?

Related: !106 (closed) adds N() and M() to the IdentityMatrix class.

In my opinion x.size() is the idiomatic way to get the size of a container. This does at least cover the number of rows for dense vectors and matrices (because we consider them to be containers of rows).

Interfaces for sparse (indexed) containers have not really been discussed by now. IMO there are two ways of thinking of them:

• A container of (index,value) pairs. Here x.size()should indeed be the number of stored entries and iterators should iterate over those pairs.
• A container which has no storage for some values. Non-stored values conceptually represent a certain default value. Here x.size() should IMHO be the conceptual size of the container. Iterators may only visit the stored entries and provide additional information on which entry they point to, e.g., the index. In contrast to the first variant this one allows for a common interface of dense and sparse containers and is btw the approach that we have taken for the already present sparse containers: matrix rows.

That said we should for consistency add x.size() to all containers. Even for the number of rows in a matrix and the number of columns in a sparse matrix' row. In view of the rather generic names x.N() and x.M() I would prefer to aditionally have x.rows() and x.cols() for matrices.

There is a similar solution as ScalarVectorView experimented with in !1354, i.e., a way to transform a given vector into a unified DenseVector interface. I stumbled upon this issue when trying to replace LFEMatrix in dune-localfunctions with DynamicMatrix and using .solve with std::vector as arguments (inverting a mass matrix)

This issue popped up multiple times now.

Matrix m{...}; // e.g. FieldMatrix or DynamicMatrix
m.mv(vecIn, vecOut);

In the implementation, we currently require that vecIn and vecOut implement the DenseVector interface, e.g., provide .N() to retrive the number of entries and operator[](std::size_t i) to access the ith entry. Since we are dealing with dense matrices, the number of entries must represent the size of the vector and the ith entry must be the ith vector component. Sparse vectors are currently not be supported by the implementation.

In several applications, I want to use something for vecIn or vecOut that is not a DenseVector. One reason is, that we do not have containers that support anything other than field-types as entries. For the matrix-vector product we actually only need the property that the entries of the vector can be multiplied by the entries of the matrix and summed up. This is much weaker.

So, I suggest to change in the assert statements the .N() to .size().

mentioned in issue staging/dune-functions#89 (closed)

mentioned in merge request !1514