Fe br

[bangerth]

No description provided.

[bangerth]

Looks overall pretty good. Just a few comments. Would you like to extract the parts that extend the mapping_type to a vector and submit that individually? These things are often easier to get through patch review in smaller increments.

[bangerth]

In essence, what you're doing here is determine whether any of the elements of the vector are different from mapping_none. (And similarly in the next few lines.) You can write this more concisely in the following way (see https://en.cppreference.com/w/cpp/algorithm/find):

  const book update_transformed_shape_values = (std::find_if_not (this->mapping_type.begin(),
                                                                                                 this->mapping_type.begin(),
                                                                                                 [](const MappingType t) { return t != mapping_none; })
                                                                                   != this->mapping_type.end());

[GrahamBenHarper]

This command is a mouthful. Let me see... so this command locates any entry between begin or end satisfies the inline function (what's the C++ word for that?) that returns true if the mapping type is not mapping_none. I assume the default return is .end() if nothing is found?

 const bool update_transformed_shape_values = (std::find_if (this->mapping_type.begin(),
                                                             this->mapping_type.end(),
                                                             [](const MappingType t) { return t != mapping_none; })
                                                                                   != this->mapping_type.end());

Should that be .end() on the second line? Also, if we use return t != mapping_none, shouldn't we use find_if instead of find_if_not?

And if I were to do this for transformed_shape_grads, would it look like this?

 const bool update_transformed_shape_values = (std::find_if (this->mapping_type.begin(),
                                                             this->mapping_type.end(),
                                                             [](const MappingType t) { return (t == mapping_raviart_thomas ||
                                                                                               t == mapping_piola ||
                                                                                               t == mapping_nedelec ||
                                                                                               t == mapping_contravariant); })
                                                                                   != this->mapping_type.end());

If so, then I think I understand how this works and I can implement it.

[GrahamBenHarper]

Oh is that called a "lambda expression"? This is something I haven't explored much of before.

[bangerth]

Yes:

• That's a lambda function -- a function without a name defined in the place where you need it.
• And yes, correct, it should have been .end() in the second line.
• And yes, if find_if can't find an entry that satisfies the lambda function, then it returns .end().

[bangerth]

https://en.wikipedia.org/wiki/Anonymous_function#C%2B%2B_%28since_C%2B%2B11%29

[bangerth]

This would work with find_if (...).

[bangerth]

This is the exact same condition as you had above for update_transformed_shape_values, so you will get the exact same answer. Is this on purpose?

[GrahamBenHarper]

Ah it was not on purpose. Seeing the original command looked like

            if (mapping_type != mapping_none)
              data->untransformed_shape_hessian_tensors.resize(n_q_points);

I can just absorb that boolean as being equal to the update_transformed_shape_values boolean since both only check if the mapping is not mapping_none.

[bangerth]

I believe you'll have to update that, right?

[GrahamBenHarper]

Yes, I will revert it to {mapping_none} once the fill_fe_values overload is finished.

[bangerth]

add the following:

  else
    Assert (false, ExcNotImplemented());

to make sure that if the code is ever used for anything other than 2d and 3d, you will be notified that that case has not been implemented.

[GrahamBenHarper]

On line 53 I have Assert(dim == 2 || dim == 3, ExcImpossibleInDim(dim));

Should I still add this one as well?

[bangerth]

Yes, I think that would be useful. The assertion at the top is an expression of the general idea that this function isn't implemented for certain cases, whereas adding the assertion here would point to a specific place where could would need to be augmented if one wanted to extend the function.

[GrahamBenHarper]

Okay got it!

[bangerth]

I think you could do it with this template argument as well.

[GrahamBenHarper]

I only hesitated because on line 73 there's a const unsigned int spacedim = 2;

Should I still throw that template in there?

[bangerth]

Just remove that line. I don't think that the function should behave differently based on the space dimension; only the dimension should matter.

[GrahamBenHarper]

Okay I updated that. Would I need to update any other files since I'm turning this into a templated function? I know there are .inst files and they relate with templates but I'm unsure how those things work.

[bangerth]

No, the function is used only in this one file. The compiler can figure this out. It only gets complicated when you declare a template in one (.h) file, define (i.e., implement) it in a .cc file, and want to use it in another .cc file.

[bangerth]

yes, just like you do here

[bangerth]

It's not wrong to use the ?: operator in an if clause, but it's often easier to read to write things out:

  if ((cond && expr1) || (!cond && expr2))

[bangerth]

Or here maybe:

  if ((mapping_type.size() > 1 ? mapping_type[cell_j] : mapping_type[0]) == mapping_raviart_thomas)

This probably expresses most clearly what you are doing.

[GrahamBenHarper]

Yes I like that second option.

[bangerth]

This is clearer now. I wonder whether the two functions could be merged into one, though I'd leave this for a follow-up patch.

[GrahamBenHarper]

I thought of that when I saw these side-by-side. If these were merged into one function, it would likely need to be a function that handles rt, nedelec, and is easy to modify in case future elements require something similar.

[bangerth]

Yes. Leave it as is for the moment. We can always come back to it.

[bangerth]

here and below, mark this variable as const since it won't change in this block.

[GrahamBenHarper]

Got it!

[bangerth]

Apologies for taking this long to get to looking at your changes. Let me know if you want me to read through them again!