Make FieldTraits less greedy

The idea of the FieldTraits class is to provide the field_type and real_type of various types representing numbers, vectors, ... . So far there was a default implementation

template<class T>
struct FieldTraits
{
  typedef T field_type;
  typedef T real_type;
};

This does not make much sense, for types that do not represent numbers. The patch ensures that the default implementation does only provide the typedefs if Dune::IsNumber<T> is true and that it is empty otherwise.

This also fixes a seemingly unrelated issue: If you defined operator*(T,T) and dot(T,T) for your own type (or concept) T, then dot is ambiguous, because there's a very greedy implementation in dotproduct.hh.

Here is a list (thanks to @simon.praetorius for the collection...) of wrong usage/missing specializations which must be fixed before merging:

• dune-istl: dune/istl/multitypeblockvector.hh:80 if no types are given or there is no common field-type of all the components, set as field_type the fallback Dune::Std::nonesuch. This results in an error, but occurs in recursive implementations because the fallback field_type is used to define a real_type. This is not correct and a corresponding nonsuch should also be defined as real_type instead. (dune-istl!480 (merged))
• dune-localfunctions: dune/istl/multiindex.hh: a type parametrized with Field type. This should be the definition in FieldTraits (dune-localfunctions!217 (merged))
• dune-localfunctions: dune/istl/tensor.hh: The LFETensor is a container type, thus should recursively define the FieldTraits. Also, there is the class Derivative defining also a field type. (dune-localfunctions!217 (merged))
• dune-common: dune/common/debugalign.hh: Need specialization for IsNumber for AlignedNumber
• dune-common: SimdArray is not detected as a valid field type anymore.
• dune-common: dune/common/fvector.hh:323: Constructor leads to second order substitution failures and thus a compile error when compiling dune-spgrid tests
• dune-istl: Also the Dune::FieldTraits<Vc_1::SimdArray<...>> needs a specialization.

assigned to @christi

@christi Please check the patch (as the original author of this class).

The change looks good.

(except for the multiprecision errors...)

added 1 commit

• c3cffc51 - Make FieldTraits less greedy

Compare with previous version

changed the description

It turned out, that std::is_arithmetic is to narrow and IsNumber is more reasonable.

There's a severe issue here: It's unclear how to define FieldTraits for the expression templates generated by GMPField. This indirectly leads to a test failure due to a strange SFINAE guard in FieldVector:

template<class K>
class FieldVector<K, 1>
...
template<typename T,
         typename EnableIf = typename std::enable_if<
           std::is_convertible<T, K>::value &&
           ! std::is_base_of<DenseVector<typename FieldTraits<T>::field_type>, K
         >::value>::type>
FieldVector (const T& k) : _data(k) {}

As a consequence construction from an expression x+y where x and y are GMPField does not work.

Why is this SFINAE check strange? It only makes sense if you are using nested FieldVector or DenseVector classes. This was never supported by dune as one can see in the blocklevel=1 definition in DenseVector. It turns out that this was introduced in bed4e242 together with a test for nested FieldVectors.

see !1387 (merged) for a possible cleanuo of this.

The leads to a chain of questions:

• Should FieldTraits be defined for any type, even if it does not make sense at all?
• If not, should FieldTraits be defined for the GMPField expression templates? How could we do that?
• If not, should the check use FieldTraits<AutononoumsValue<T>>? This seems to be reasonable, but AutonomousValue is also not specialized for the GMPField expressions.

An alternative that would also fix the specific test failure:

• Should the SFINAE check be removed, since we do not supported nested (Field|Dense)Vectors? This would require to also remove the nested checks in dune/common/test/fvectorconversion1d.cc.

In principle I would say 'yes', because we once explicitly decided to not support this and removed nested FieldVectors from the dune-localfunctions interface. But I fear, that people are using it (although it's only half functional).

@gruenich Was it intended to use nested FVs when merging !735 (merged)?

No, I just merged the added test from Claus-Justus Heine. It fixed a problem for him and looked good to me. I have no opinion on nested FVs.

Removing the nested FVs from dune-localfunction was for me never a decision to disallow nested fv in general. I am not quite sure if we are using them in dune-fem but removing the option is clearly a change in functionality in probably the most central part of Dune which should be done very carefully if at all.

Do you mean with half functional only the issue with the blocklevel or is there more?

@andreas.dedner There's a lot: E.g. blocklevel==1, dim(), and the norms of general (Field|Dense)Vectors and inequality comparison operators, operator/ for the FieldVector<K,1> specialization.

BTW: This SFINAE check goes back to 9d46150e where @christi introduced it with std::is_same instead of std::is_base_of. This check also only makes sense for nested FVs. But looking at the commit message it seems that T and K are mixed up in this check.

BTW: This SFINAE check goes back to 9d46150e where @christi introduced it with std::is_same instead of std::is_base_of. This check also only makes sense for nested FVs. But looking at the commit message it seems that T and K are mixed up in this check.

I'll have to have a look, but first I have to teach...

Any feedback on this?

I have to admit, that I don't recall how this was intended. The log message does not really fit to the code.

I suggest to rewrite this constructor to make reasoning about the overload easier.

See !1387 (merged) for a draft

added 3 commits

• 382ad230 - Add a check if FieldTraits is empty
• a084fb89 - [test][bugfix] Define FieldTraits for test implementation of a DenseVector
• 107cecdd - Add FieldVector<K,1> constructor for expression templates

Compare with previous version

I pushed an updated version that keeps the nested FV/DV construction and corresponding test at least as functional as before. More precisely:

• Add a check for empty FieldTraits to catch expression templates. (Not exposed publicly for now.)
• Add another FieldVector<K,1> constructor constrained by the new check.
• Change SFINAE pattern, because the existing one does not allow to have multiple constrained overloads. (That's a known restriction of type based SFINAE.)
• Implement missing FieldTraits for test vector implementation (see below).

While doing this I noticed that you have to define both,DenseMatVecTraits and FieldTraits, when deriving from DenseVector. That's a pity and could probably be improved. But that's another issue. The current MR just changes a broken FieldTraits<T>::field_type into a non-existing one.

added 1 commit

• f1598504 - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

Any objections to merging this? Especially: @andreas.dedner I tried to keep nested FieldVectors as functional as before, but I don't know if this imposes downstream errors for your code. @christi This changes the constructor you once introduces and I don't understand what it was supposes to do originally.

@carsten.graeser: Seems like a very useful patch but since it's at a central place some testing is needed. This may take a week or so. Is there any (simple) chance for deprecation, i.e. keep the old way in place with a warning if used?

I have no idea how this could be deprecated. The problem is that downstream code used FieldTraits in a "wrong" way, by relying on FieldTraits<T>::field_type in cases where it does not make sense at all. The accompanying patches fix such places in dune-common. But we have to check other places carefully, especially those where FieldVector is used in a nested way (which I assumes was abandoned long ago).

BTW: Even with the patch I think that the offending FieldVector constructor (to be precise: It's SFINAE checks) do not make sense.

We could introduce a macro for backwards compatibility, that provides the old code. If the macro is not set (by default), the new code is used. The macro could be removed together with old code after the next release.

This patch breaks some code in dune-fem: dune-fem/dune-fem!488 (closed) . I have to look what the exact problem is. But it shows that this might also be the case in other places.

added 40 commits

• f1598504...a85c317c - 36 commits from branch master
• 8f5ce47b - Make FieldTraits less greedy
• ab649c1f - Add a check if FieldTraits is empty
• 97093157 - [test][bugfix] Define FieldTraits for test implementation of a DenseVector
• 0127716a - Add FieldVector<K,1> constructor for expression templates

Compare with previous version

The remaining problem occurring in dune-fem comes from types in dune-localfunction, e.g.

dune-common/dune/common/fvector.hh:50:49: error: no type named 'field_type' in 'struct Dune::FieldTraits<Dune::LFETensor<double, 2, 1> >'
   50 |     typedef typename FieldTraits<K>::field_type field_type;
      |                                                 ^~~~~~~~~~
dune-common/dune/common/fvector.hh:51:48: error: no type named 'real_type' in 'struct Dune::FieldTraits<Dune::LFETensor<double, 2, 1> >'
   51 |     typedef typename FieldTraits<K>::real_type real_type;

Not sure what the fix for that should be and how many other places in dune-localfunctions and other modules will be affected by this change, which took away the default implementation of FieldTraits.

mentioned in merge request !1075 (merged)

In https://gitlab.dune-project.org/santiago.ospina/dune-concepts/-/merge_requests/12 I tried to more precisely define what a field type is and what the FieldTraits should provide.

Deactivating the default implementation of FieldTraits reveals (in my opinion) several wrong implementations/bugs, e.g. in dune-localfunctions and maybe in other modules. These should be investigated before we can merge this MR.

added 107 commits

• 0127716a...0a920260 - 102 commits from branch master
• 267d5324 - Make FieldTraits less greedy
• 330943df - Add a check if FieldTraits is empty
• 43894ecd - [test][bugfix] Define FieldTraits for test implementation of a DenseVector
• 05cb11e5 - Add FieldVector<K,1> constructor for expression templates
• 9bca5b11 - Specialize IsNumber for AlignedNumber type

Compare with previous version

changed the description

mentioned in merge request dune-istl!564 (merged)

After the issue with dune-istl!564 (merged) showed up, I want to look into this MR again. Proposal:

1. Go through the usage of FieldTraits in the core/staging/extensions modules to find misuses.
2. Either replace misuses with something more meaningful, or provide a specialization for FieldTraits if necessary
3. (optional) Investigate the cases where a default type FieldTraits::field_type is meaningful
4. (optional) Think about meaningful default specializations for FieldTraits, e.g. if the container provides ::field_type

Also, I was thinking: nested FieldVector, or FieldVector with some non-number type "seem" to work, so what are the formal requirements we actually mean by "field". Often we think about something that can be added, multiplied, initialized by 0,... maybe we need more. Is a GMP expression supposed to be a "field" in that context, or is it just something that can be converted into a number type?

On the other hand, I was thinking: What are we using the FieldTraits for, i.e., what is its meaning: Often we mean the leaf-element-type in a nested container. Then we do not need to talk about "fields", but just a formal way to extract that element type.

Also, I was thinking: nested FieldVector, or FieldVector with some non-number type "seem" to work, so what are the formal requirements we actually mean by "field".

They actually do not work, they just compile if you don't use and instantiate the failing methods.

Often we mean the leaf-element-type in a nested container.

If we only need a statically sized container of uniform type, we should use std::array.

I made a rebase to master, such that the related MRs compile again.

Maybe I have done a mistake during rebase. I have the impression that the FieldVector constructor is still not correct.

I have added the || IsNumber<T>::value condition to the FieldVector constructor and removed the other one from scalar values. Still it does not compile for spgrid, due to a second-order substitution failure. Maybe with !1387 (merged) this could be solved without any usage of FieldTraits in the constructors of FieldVector anymore. At least it seems to compile the dune-spgrid tests

added 665 commits

• 9bca5b11...adb7d071 - 661 commits from branch master
• 0624b51b - Make FieldTraits less greedy
• 46961e8a - Add a check if FieldTraits is empty
• f36078c8 - [test][bugfix] Define FieldTraits for test implementation of a DenseVector
• 397ec234 - Add FieldVector<K,1> constructor for expression templates

Compare with previous version

marked the checklist item dune-localfunctions: dune/istl/multiindex.hh: a type parametrized with Field type. This should be the definition in FieldTraits (dune-localfunctions!217 (merged)) as completed

marked the checklist item dune-localfunctions: dune/istl/tensor.hh: The LFETensor is a container type, thus should recursively define the FieldTraits. Also, there is the class Derivative defining also a field type. (dune-localfunctions!217 (merged)) as completed

marked the checklist item dune-istl: dune/istl/multitypeblockvector.hh:80 if no types are given or there is no common field-type of all the components, set as field_type the fallback Dune::Std::nonesuch. This results in an error, but occurs in recursive implementations because the fallback field_type is used to define a real_type. This is not correct and a corresponding nonsuch should also be defined as real_type instead. (dune-istl!480 (merged)) as completed

added 1 commit

• 33b368af - Rewrite the FieldVector<1> constructor from scalar values

Compare with previous version

mentioned in merge request !1387 (merged)

mentioned in merge request dune-istl!480 (merged)