Rework DenseMatrix assignment

This is meant as a fix for bug #5 (closed).

It

• Enables assignment across different DenseMatrix implementations, e.g. assignment from a DynamicMatrix to a FieldMatrix (this previously either compiled and segfaulted or did not compile)
• Catches invalid FieldMatrix assignments at compile-time
• Retaines the DenseMatrixAssigner class in order to remain compatible with user-defined matrix classes

To me it looks 99% good to go. There is only remaining question that I cannot answer on my own:

• Should FieldMatrix<double, 3, 3> x = 27 compile? In other words, should the constructor be marked as explicit or not? Of course, one can always use FieldMatrix<double, 3, 3> x(27) instead.

I've tested this fix with multiple modules and it doesn't break anything for me.

mentioned in issue #5 (closed)

This merge request should maybe be combined with !48 (closed).

Added 1 commit:

• 22eaa3fc - IdentityMatrix: Test assignment to DenseMatrix

Added 1 commit:

• 3aa11e69 - IdentityMatrix: Test assignment to DenseMatrix

The one nasty thing that I'm not happy about is (I already mentioned this in #5 (closed), @carsten.graeser will remember):

Dune::DynamicMatrix<double> d(3, 3);
d = Dune::DynamicMatrix<double, 2, 2>{{1,2},{3,4}}; // needs !48 to compile

will resize d to a 2x2 matrix and set its values. That's how it's always been.

Dune::DynamicMatrix<double> d(3, 3);
d = Dune::FieldMatrix<double, 2, 2>{{1,2},{3,4}};

will resize d to a 2x2 matrix and set its values. That's new, it's analogous to the above, all good.

BUT

Dune::DynamicMatrix<double> d(3, 3);
d = Dune::FieldMatrix<double, 1, 1>{{1}};

will not resize d, since it is identical to

Dune::DynamicMatrix<double> d(3, 3);
d = 1;

because Dune::FieldMatrix is convertible to a scalar. This is something that's always happened but in the past it wasn't so confusing.

Added 4 commits:

• 8dffc195 - Rework assignment & construction of dense matrices
• 2ca760da - IdentityMatrix: Rename template parameter
• bd6cff06 - IdentityMatrix: Add N() and M()
• b54ce93d - IdentityMatrix: Test assignment to DenseMatrix

Should I open a separate merge request for the changes to IdentityMatrix?

@carsten.graeser, @christi: Pinging you here because you've shown interest in this matter in the past :)

This issue has lain dormant for 3 months now. I guess it will soon stop to be mergeable without conflicts. Can I interest anyone in taking another look and commenting?

I think this is a nice change. It adds all kinds of tests that should've been there to begin with. The change is not tiny but it's clear and the resulting code is finally straight-forward. It's an improvement in my book.

You mentioned the issue of

Dune::DynamicMatrix<double> d(3, 3);
d = Dune::FieldMatrix<double, 1, 1>{{1}};

Can't we solve this issue by further narrowing this assignment? We could use something like

std::enable_if_t<std::is_convertible<T, K>::value> && !isDenseMatrix<T>::value>

with an appropriate definition of isDenseMatrix<T>. One option is to match on the operator[]

Added 3 commits:

• c7a7c662 - Bug fix: Unexpected DynamicVector::resize behavior
• 46770758 - Handle and test 1x1 matrices
• fa96a007 - Treat GMPField as a scalar; test it

@christi I've now addressed that. I think the resulting behaviour is reasonable; I went with a whitelist rather than a blacklist approach, though: Everything's treated as a matrix unless it's a scalar. If it's a scalar is determined by Dune::is_scalar, which is a superset of std::is_scalar. So far, I've only made it to include GMPField. Is there something that I've missed?

Added 1 commit:

• c95ead20 - Use Dune::is_scalar consistently

I like the concept of Dune::is_scalar

What is missing is std::complex, std::is_scalar states "no", but we use it as a scalar value...

That said... perhaps something like Dune::isField might be better?!

I chose the name because I wanted to build upon std::is_scalar but now that I've taken a closer look, I find that I completely misunderstood the idea behind that trait: Any pointer is a scalar, too, e.g.. The proper name for the trait that I had in mind is std::is_arithmetic (a subset of std::is_scalar). That contains all the integral and floating point types (but nothing else).

I'd thus like to suggest Dune::is_arithmetic as an extension of std::is_arithmetic. If you prefer Dune::isArithmetic or even Dune::isField (please note though that many of the types covered by it wouldn't form a field), I'm fine with that, too. Dune::is_primitive or Dune::isPrimitive would be another option. Maybe it's not even desirable to "extend" the std:: type the way that I'm doing it because one might expected Dune::is_arithmetic and std::is_arithmetic to be identical -- we've been providing such drop-in replacements after all.

The lack of support for complex types was an oversight, thanks for pointing that out. I'll get on that now. Update: taken care of.

Added 1 commit:

• ba629ef5 - Treat complex types as scalars; test them

@christi: Ping.

Added 1 commit:

• c555376d - Rename is_scalar to isNumber, extend std::is_arithmetic

mentioned in merge request !105 (merged)

Added 7 commits:

• 194b1ea1 - Add isNumber trait, extending std::is_arithmetic
• 7de2caac - Rework assignment & construction of dense matrices
• 620b1ea3 - IdentityMatrix: Rename template parameter
• 83d9a21b - IdentityMatrix: Add N() and M()
• 7e5b15ad - IdentityMatrix: Test assignment to DenseMatrix
• d3e6dc80 - Bug fix: Unexpected DynamicVector::resize behavior
• 0c0d08be - Handle and test 1x1 matrices