Combining bndLeastSqConservedVar() and bndLeastSqPrimitiveVar()

According to @jbakosi, in PR https://github.com/quinoacomputing/quinoa/pull/348 comment: Now that I'm thinking about these two (bndLeastSqConservedVar and bndLeastSqPrimitiveVar) a little more, and looking the at the diff between them, there is now a lot of code repeated.

I think something like what was previously, using a single function, would be better. (Sorry, I'm going back and forth with this -- this is how my understand increases for what we are doing here and what can be done to write this a little better.) Here is what I think might be able to capture the best of both worlds (achieving code-reuse as well as being readable):

There would be a single function.
There would be some function argument, say u, that differentiates between whether the function was called for conserved or primitive variables. This could be a bool, but probably more readable if we make it an enum with named members CONSERVATIVE and PRIMITIVE.

Based on 2 above, we could assign a callable, which would store which version of the following code needs to be executed inside the loop:

   auto ul = eval_state( ncomp, offset, rdof, 1, el, U, B );
   std::vector< real >uprim(nprim,0.0);
    // consolidate primitives into state vector
    ul.insert(ul.end(), uprim.begin(), uprim.end());

    auto ul = eval_state( nprim, offset, rdof, 1, el, P, B );
    std::vector< real >ucomp(ncomp,0.0);
    // consolidate conserved quantities into state vector
    ul.insert(ul.begin(), ucomp.begin(), ucomp.end());

Then we would first create two lambdas, then assign a callable:

// Note that std::vector::insert below uses the simpler overload (3) at
// https://en.cppreference.com/w/cpp/container/vector/insert instead overload
// (4) with a temporary vector. (I *think* it should do the same, but I have not tested this.)

auto cons = [&]( std::size_t el ){
  auto ul = eval_state( ncomp, offset, rdof, 1, el, U, B );
  ul.insert( end(ul), nprim, 0.0 );
  return ul;
}

auto prim = [&]( std::size_t el ){
  auto ul = eval_state( nprim, offset, rdof, 1, el, P, B );
  ul.insert( begin(ul), ncomp, 0.0 );
  return ul;
}

auto left_state = (u == CONSERVATIVE ? cons : prim);

Then below this, we would call this as

std::vector< real >B(1,1.0);
auto ul = left_state( el );

This solution would separate out the difference in the beginning of the function outside of the main loop, so I think this would be more readable compared to the previous if test. (It would also avoid the if test, or more precisely, it would move it outside to the ternary operator, when left_state is assigned. What do you think? (A similar solution can be done for adding to rhs_ls.)

This should be done, after multimat is working and tested. Thanks for the suggestion @jbakosi.

quinoacomputing / quinoa

Combining bndLeastSqConservedVar() and bndLeastSqPrimitiveVar() #350