And thank you for changing the code.
Well, I don't fully agree. The argument is passed on to
NumLib::LocalToGlobalIndexMap const& getDOFTable(
const int process_id) const override;
where it is called process_id
.
So IMO `process_id is the better name.
Thank you for testing.
OK, good to know.
Yes, the code is correct. But it might be a surprise to see the size changing all the time. But it's OK IMO.
Model of permeabilities computation is extracted and the data is stored in "output"-data.
Small cleanup of an permeability model; total_strain
variable is removed, as it is not used (nor is it tested).
Review commit-wise.
Model evaluation moved upwards. I hope that won't break anything.
wouldn't simple assignment work?
total_stress = rhs
?
If rhs is a concrete type, not an Eigen expression, it should work.
I'm through. The code looks correct to me. Now or in the future we should reconsider if the glue code lambdas used everywhere, now, are good.
And some data will be copied from Eigen::Map
to Eigen::VectorXd
.
I'll be on vacation next week. You don't have to wait for a second review from my side.
Dima already suggested that.
auto get_a_dof_table_func = [this](const int process_id) -> auto&
{
return getDOFTable(process_id);
};
several locations in several commits.
Btw., shouldn't the argument be called process_id
?
The need for -> auto&
to avoid a copy can be easily forgotten. Maybe using an ad hoc lambda is not the greatest idea. I'm unsure :-/
Somehow it feels wrong that we need to introduce such a piece of glue code to be able to use the standalone function. Now, we need glue code twice in the RM process. Before, we had one separate method getDOFTables()
. IMO the situation before was better than now. IMO this indicates that the organization of DOF tables in the processes is suboptimal. Maybe putting DOF tables in a separate struct that handles access to them would be a good solution. But that's probably for a separate MR.
This could be a standalone function, now, as far as I can see. DOFTableUtil.h might be a good location for it. Maybe there already is such a functionality.
same bind
thingy.
Will this function be called several times in a staggered setting? If so, is that meaningful?
AFAIK using std::bind
is discouraged, now. See e.g. https://en.cppreference.com/w/cpp/utility/functional/bind_front:
These function templates are intended to replace std::bind. ...
An ad hoc lambda might be more readable than bind: [this](int process_id){ return getDOFTable(process_id); }
.
Note: In this call the Eigen::Map data will be copied to an Eigen::VectorXd!
Are you planning to do this for other methods as well, later on? assemble(), preTimestep() etc?