Constrain dot templates to exclude meaningless cases

This constraints the non-vector overload of Dune::dot(a,b) to cases where the first argument is a number in the sense of Dune::IsNumber. This helps to avoid ambiguity if one wants to implement both, dot(a,b) and a*b manually for a custom type. Without this patch, the availability of a*b activates Dune::dot(a,b) concurrently to the custom implementation.

This also constrains transpose<M>(...) to classes that at least provide M::row_type.

Both are just partial fixes of a deeper problem, but a full fix requires a severe refactorization of the matrix/vector interface.

added bugfix label

I don't see any reason for implementing a generic Dune::dot(a,b) such that the result is not a number. In fact there are use cases of dot(a,b) where this assumption is false (e.g. expression templates). However, you would not want to use the generic one, but implement a custom dot method in this case anyway - which is made possible with this patch.

Thus I consider this a bugfix of the overly greedy generic implementation. Any objects to merging this (... if is does not break anything in the core)?

I agree, the generic implementation Dune:dot should not prevent from implementing your own dot-product or multiplication. The current implementation is to some extend questionable, but whatever...

Just one comment: With your change there are now multiple enable_if in one function definition: in the template declaration and in the return type specification. This makes it very hard to read. Maybe one could just merge these two constraints, or at least write it in one style, e.g.,

  template<class A, class B,
    std::enable_if_t<
      !IsVector<A>::value &&
      !std::is_same<typename FieldTraits<A>::field_type, typename FieldTraits<A>::real_type>::value &&
      Dune::IsNumber<decltype(conj(std::declval<A>())*std::declval<B>())>::value, int> = 0>
  auto dot(const A & a, const B & b) -> decltype(conj(a)*b)
  {
    return conj(a)*b;
  }

ok... not much better. Maybe you have a better idea.

One should probably reimplement the whole file in a different style with a proper specification of the requirements.

just an experiment: https://gitlab.dune-project.org/-/snippets/60

My original approach does not work, because isNumber is not defined to true for the GMPField expression templates. I pushed another one that is less invasive:

So far the code is based on the assumption that a type T is a vector if T::field_type is defined. The offending overloads are the ones for the non-vector case. Assuming that not being a vector implies being a number, I now added the addional constraint that IsNumber<T> is true. This solves my issue. Furthermore, in contrast to my first approach, the types passed to the function are under control.

Potential issue: Do we need to allow passing GMPField expression templates as arguments?

Even if this patch is not the desired solution, I really think that this should be fixed. Having catch-all templates for generic names like dot and transpose that rely on a fixed implementation should be avoided. Either we restrict the functions to types under control (FieldTraits<T> greatly fails in doing this, cf !1040) or we at least provide a reasonable customization point.

I agree! If this minimalistic patch works, we should merge it.

A discussion about a proper linear algebra interface and function definition should be done some time, e.g. in a developer meeting.

Notice that this should be carefully tested with downstream modules (especially due to the dirty abuse of FieldVector even in the core).

I also added a slight improvement for transpose. And fixed the CI-problem in dune-istal which was due to a missing specialization of IsNumber<AlignedNumber>.

The downstream CI-failurein dune-geometry was due to an unrelated svg-problem.

This still fails due to a missing specialization of IsNumber while dune/common/simd.vc provides

template <typename T, std::size_t N>
struct IsNumber<Vc::SimdArray<T, N>>
  : public std::integral_constant<bool, IsNumber<T>::value> {
};

the multirhstest.cc in dune-istl also uses Vc::SimdArray<double,2,Vc::Vector<double, Vc::VectorAbi::Scalar>,1>. Let's try adding the other two template arguments in IsNumber.

BTW: Its strange that multirhstest.cc also specifies the last template parameter called Wt in the documentation, because the latter explicitly says:

Wt: Don't ever change the default value. This parameter is an unfortunate implementation detail shining through.

But I never used this and have no idea what this parameter means and will not provide a patch but leave this to the experts.

The downstream dune-istl problem is fixed. But the unrelated CI issue with dune-geometry persists:

CMake Error at doc/appl/refelements/CMakeLists.txt:5 (dune_create_inkscape_image_converter_target): Unknown CMake command "dune_create_inkscape_image_converter_target". -- Configuring incomplete, errors occurred!

Some time ago, @tkoch has improved the documentation generation and has added a function in dune-common (!1065 (merged)) that is used in dune-geometry (dune-geometry!187 (merged)). Not sure why this is not used in the system test. Maybe you have to rebase on the latest master of dune-common?

I just found this myself and rebased the branch on master.

The 'nightly' downstream pipeline passes now. I additionally tested with some more downstream modules (dune-functions, dune-fufem, dune-alugrid, dune-uggrid, ...) and all of them pass. Dune-pdelab CI fails due to unrelated problems (and it currently also does with master).

I started a system test, see https://gitlab.dune-project.org/infrastructure/dune-nightly-test/-/pipelines/42241

It fails only in the PDELab repository as mentioned before. I also tested with AMDiS, without errors. Thus, it looks good to me to be merged.

One problem: if you multiple GMPField types, you will not get a GMPField, but an expression. This does not support IsNumber.

added 1 commit

• 071a5d16 - Constrain dot to cases where it makes sense

Compare with previous version

changed the description

added 2 commits

• e0b806ef - [bugfix] Add missing spezialization of IsNumber<AlignedNumber>
• 21b3a5e3 - Constrain the free transpose<M>() function

Compare with previous version

changed title from Constrain dot and dotT cases to where the result is a number to Constrain dot and transpose templates to exclude meaningless cases

changed the description

added 1 commit

• 9b0a5ed1 - Specify all template parameters in `IsNumber<SimdArray<...>>`

Compare with previous version

added 62 commits

• 9b0a5ed1...d79557a1 - 58 commits from branch master
• 219c2b0a - Constrain dot to cases where it makes sense
• 6338e8ab - [bugfix] Add missing spezialization of IsNumber<AlignedNumber>
• 601960b7 - Constrain the free transpose<M>() function
• fdba2fb9 - Specify all template parameters in `IsNumber<SimdArray<...>>`

Compare with previous version

ok, this is now discussed a lot and tested in multiple modules. Maybe we should merge it now.

The only thing I don't like is that the changes in transpose() are mixed into this MR. This is also changed in other MRs and will result in merge conflicts. It is also somewhat unrelated to the fix of dot().

@carsten.graeser Since you also change the implementation of transpose() in other MRs. Should this be removed from this MR here? Or is this compatible. If it is compatible, I would merge this MR.

There's !1138 (merged). This one also has the check for Matrix::row_type. So we should probably remove it here.

I removed the patch concerning transpose() and updated the description. Thanks for noting this.

added 130 commits

• fdba2fb9...9ea19652 - 127 commits from branch master
• 9ef93022 - Constrain dot to cases where it makes sense
• 5298178b - [bugfix] Add missing spezialization of IsNumber<AlignedNumber>
• 800dc75a - Specify all template parameters in `IsNumber<SimdArray<...>>`

Compare with previous version

changed title from Constrain dot and transpose templates to exclude meaningless cases to Constrain dot templates to exclude meaningless cases

changed the description

ok, I will merge now. Thanks for the bugfix.

mentioned in commit e8aec08f

merged