-
Notifications
You must be signed in to change notification settings - Fork 707
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add utility functions to evaluate values of arbitrary (remote) points #11804
Add utility functions to evaluate values of arbitrary (remote) points #11804
Conversation
b522038
to
479c5d3
Compare
This basically re-implements With the existing functionality, this could have been implemented in the following way: LA::MPI::Vec global_vec;
...
FiniteElement<dim,spacedim> &fe = ...;
...
std::vector<Point<spacedim>> points(n_points);
std::vector<double> values(n_points);
auto global_boxes = cache.get_covering_rtree(tree_level);
auto [cells, local_points, local_indices, global_points, from_cpu] = distributed_compute_point_locations(points, cache, {global_boxes.begin(), global_boxes.end()});
for(unsigned int i=0; i<cells.size(); ++i) {
const auto &cell = cells[i];
const auto &qpoints = local_points[i];
const auto &indices= local_indices[i];
for(unsigned int j=0; j<indices.size(); ++j)
values[indices[j]] = fe.value(qpoints[j]);
}
// now send around the `values` vector
... I think your implementation is likely to be more efficient, but can we at least gather functions that do the same stuff around with just one implementation (i.e., the one that works best), and throw away the others? |
@luca-heltai you are right, we should try to unify the implementations and have a good infrastructure that fits a wide application range. I will look into this more closely tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some various comments that I would like to discuss.
namespace EvaluationFlags | ||
{ | ||
enum EvaluationFlags | ||
{ | ||
avg = 0, | ||
max = 1, | ||
min = 2, | ||
insert = 3 | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we select a better name here, e.g., EvaluateReductionMethod
, and document the intent of this class as the combination method for the case several function values are present in the given point (e.g. discontinuous Galerkin).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before making this change, let's agree on how to deal with gradients
, hessians
! Is it an option that we have for each a separate function as stated a couple of minutes ago!? Or do we want to encrypt this in these "flags". An issue I seen in the latter is that we don't have a way to determine the return type of the function based on the flag so that the user might need to be explicit regarding the template argument!?
std::vector<typename FEPointEvaluation<n_components, dim>::value_type> | ||
evaluate_at_points( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also think about the case where a user requests the gradient of a solution field (or the Hessian). How would we structure the case of various return kinds? Set up some additional data structure that is filled upon demand? In terms of technicalities, I think we can return any information FEPointEvaluation
can compute, as long as we can pack the data appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of technicalities, I think we can return any information FEPointEvaluation can compute, as long as we can pack the data appropriately.
Yes that is right. I would suggest once other quantities are needed, we move the content of this function to an internal (templated) function, which is called by evaluate_values_at_points()
, evaluate_gradients_at_points()
, ...
I think we should also think about the case where a user requests the gradient of a solution field (or the Hessian). How would we structure the case of various return kinds? Set up some additional data structure that is filled upon demand?
The bigger question is how we can deal with derived quantities (e.g. in the case of SIM I would like to communicate already the force computed with JxW
so that only has to be tested after communication) and with composites (e.g., you might want to pack multiple quantities (one might be a scalar, the other one vectorial) together so that they are sent in one go; e.g. BDF-2 multiple timesteps after AMR step).
The best solution I was able to come up (in adaflo) for this till now is use RemovePointEvaluation
directly. This class can work with arbitrary types so that I can also pass in vectors of tuples. What is your opinion?
template <int n_components, int dim, int spacedim, typename VectorType> | ||
std::vector<typename FEPointEvaluation<n_components, dim>::value_type> | ||
evaluate_at_points( | ||
const DoFHandler<dim, spacedim> & dof_handler, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first sight, it seemed unexpected to me to need to hand in a DoFHandler
object here, as we crucially rely on the precomputed cache
in terms of point locations and all other geometry (that is provided by DoFHandler::get_triangulation()
in the implementation below). I think the motivation is to get the interpretation of the vector, because we need to get cell iterators and iterator->get_dof_indices()
to access the solution here. So in conclusion I do not disagree here, but I think we need to work hard to document this well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is that "surprising". All VectorTools
functions get a DoFHandler
object. The DoFHandler
object is connected "directly" to the vector while the communication pattern is only connected to the grid.
So in conclusion I do not disagree here, but I think we need to work hard to document this well.
But yes we should!
const auto evaluation_point_results = [&]() { | ||
std::vector<std::unique_ptr<FEPointEvaluation<n_components, dim>>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this lambda (used once to populate a vector evaluation_point_results
) and write the code straight-out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you don't like these kind of scopes. For me, however, it helps to structure code.
template <int dim, int spacedim> | ||
const std::vector<unsigned int> & | ||
RemotePointEvaluation<dim, spacedim>::get_point_ptrs() const | ||
{ | ||
return quadrature_points_ptr; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function (and the small ones below) can go to the header file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I can do that. What is the criterion when to leave a function in the header file. I would have thought that there are only two reasons: 1) that the function is templated in such a way that explicit instantiation is hard and 2) the calls is performance critical (as is in many places of FEEvaluation
). But the functions here are definitely not performance critical so that they do not need to be inlined...
c42ae7c
to
8215af5
Compare
This statement is not exactly precise. The main use case of In contrast, the the goal of this PR it be enable evaluation/integration on arbitrary points (local and remote; potentially on non-matching meshes). A function added is To be able to perform the communication, additional data structures have to be set up. In particular, data have to be sorted in each stage differently: during evaluation data needs to be sorted according to the owning cell; during communication the data has to be sorted according to the ranks; and at the end the data has to sorted in the order of the provided points. Furthermore, this needs additional communication steps for finalizing the data structures via handshake that can be performed via Since the result of
This class can be passed to Note: I will move some minor changes to separate PRs. |
8215af5
to
1d9a6ef
Compare
1d9a6ef
to
729f093
Compare
/rebuild |
PR is ready from my side. I have renamed some of the fields and have added some documentation. The discussions above (particularity regarding usability and extensibility - computation of other quantities) are still open. I will resolve these once we have agreed on a good solution. |
Regarding the discussion above. To be able to work with dealii/include/deal.II/numerics/vector_tools_evaluate.h Lines 209 to 256 in 57169e3
The question what prevents users to write their own lamdba functions. Apart from the fact that I am dealing here with the general case (hp), I see two issues 1) a lot of boiler code and 2) the need to work with the CRS data structures directly. In the ideal case, the above code could be rewritten in the following way: const auto fu = [&](const auto &data, auto &values, const auto cell_range) {
FEPointEvaluation<n_components, dim> evaluator(data, cache.get_mapping(), dof_handler);
for (unsigned int cell = cell_range.first; cell < cell_range.second; ++cell)
{
evaluator.reinit(cell);
evaluator.read_dof_values(vector);
evaluator.evaluate(dealii::EvaluationFlags::values);
evaluator.set_point_values(values);
}
}; Adding additional evaluators in this context would be easier. But the question is if it is really worth it to move the complexity to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will write more later, but let me push what I already have.
(void)fu; | ||
#else | ||
output.resize(point_ptrs.back()); | ||
buffer.resize((send_permutation.size()) * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit-picking, but do we need the second pair of braces?
buffer.resize((send_permutation.size()) * 2); | |
buffer.resize(send_permutation.size() * 2); |
temp_map[send_ranks[i]] = Utilities::pack( | ||
std::vector<T>(buffer_2.begin() + send_ptrs[i], | ||
buffer_2.begin() + send_ptrs[i + 1])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we disable compression to keep reasonable performance?
temp_map[send_ranks[i]] = Utilities::pack( | |
std::vector<T>(buffer_2.begin() + send_ptrs[i], | |
buffer_2.begin() + send_ptrs[i + 1])); | |
temp_map[send_ranks[i]] = Utilities::pack( | |
std::vector<T>(buffer_2.begin() + send_ptrs[i], | |
buffer_2.begin() + send_ptrs[i + 1]), false); |
|
||
auto &buffer = temp_map[send_ranks[i]]; | ||
|
||
requests.resize(requests.size() + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion,
requests.resize(requests.size() + 1); | |
requests.push_back(MPI_Request()); |
would be simpler to read, but I do not feel strongly here.
MPI_STATUS_IGNORE); | ||
|
||
temp_recv_map[status.MPI_SOURCE] = | ||
Utilities::unpack<std::vector<T>>(buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we disable compression above, we of course also need to do that here.
(void)buffer; | ||
(void)evaluation_function; | ||
#else | ||
// expand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you slightly expand the comment here?
if (recv_rank == my_rank) | ||
continue; | ||
|
||
temp_map[recv_rank] = Utilities::pack(temp_recv_map[recv_rank]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
evaluate_at_points( | ||
const DoFHandler<dim, spacedim> & dof_handler, | ||
const VectorType & vector, | ||
const Utilities::MPI::RemotePointEvaluation<dim, spacedim> &cache, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been thinking about this function again and I think we should have the cache
variable as the first argument, to avoid confusion with the previous function and the implicit mapping
argument other functions use in deal.II.
: tolerance(tolerance) | ||
, ready_flag(false) | ||
{} | ||
template <int dim, int spacedim> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some empty lines her and below as usual in deal.II.
@kronbichler I have incorporated your suggestions! |
if (n_refinements >= 2) | ||
{ | ||
for (auto cell : tria.active_cell_iterators()) | ||
if (cell->is_locally_owned()) | ||
if (0.3 <= cell->center().norm() && cell->center().norm() <= 0.43) | ||
cell->set_refine_flag(); | ||
tria.execute_coarsening_and_refinement(); | ||
} | ||
|
||
if (n_refinements >= 3) | ||
{ | ||
for (auto cell : tria.active_cell_iterators()) | ||
if (cell->is_locally_owned()) | ||
if (0.335 <= cell->center().norm() && | ||
cell->center().norm() <= 0.39) | ||
cell->set_refine_flag(); | ||
tria.execute_coarsening_and_refinement(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to remove these two additional refinements - I think it's enough if you simply add some step to refine that creates sufficiently different parallel partitions in terms of the owned cells not overlapping. This keeps the test shorted (and the compactness is really the key of this class).
f03ed9e
to
6bb2e82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has reached a good state. Again, I have been involved here, so we need someone else looking into it as well.
@@ -0,0 +1,4 @@ | |||
CMAKE_MINIMUM_REQUIRED(VERSION 3.1.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you want another directory for the tests here that doesn't match the location of the corresponding source files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep it this way to be able to collect the test that use RemotePointEvaluation
in one place.
// Like remote_point_evaluation_01.cc but normal and curvature vector living on | ||
// background mesh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this different from tests/remote_point_evaluation/remote_point_evaluation_02.cc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the comment. This was just a copy&paste error.
} | ||
|
||
vector_1.print(deallog.get_file_stream()); | ||
vector_2.print(deallog.get_file_stream()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you check for correctness here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look at the result in Paraview. If you want I can replace the printing of the vectors by computing the global error for the following tests: vector_tools_evaluate_at_points_01
, vector_tools_evaluate_at_points_03
, vector_tools_evaluate_at_points_04
, vector_tools_evaluate_at_points_05
. For the other tests, it is - as far as I know - not possible to come up with some derived quantities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I would prefer to check for the global error in the tests you mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After that, it's good from my side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Done! Thanks again for the review!
MPI_COMM_WORLD); | ||
} | ||
|
||
force_vector_sharp_interface.print(deallog.get_file_stream()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you check for correctness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look at the result in Paraview. Furthermore, I use the vector as the right-hand-vector in a Navier-Stokes solver and the resulting pressure jump (the derived quantity) is correct there.
6bb2e82
to
9edfa8b
Compare
@masterleinad Thanks for the review. I have addressed most of them. I have one question in #11804 (comment). |
9edfa8b
to
6f1c7e8
Compare
The big brother of
FEPointEvaluation
.The classes/functions have enabled us to
I will add some tests tomorrow so that the use cases should be more clear.