Feature transfer_search

[ljcarlin]

This is an abstracted version of the algorithm I wrote for the GMT example.

p4est_transfer_search is a collective, point-to-point transfer for maintaining a distributed collection of points. After communication, points are stored (only) on the processes whose domains they intersect.

Points can be instances of an arbitrary struct. Point-quadrant intersection is specified by a user supplied callback. A single point may intersect multiple process domains, and after communication will be known to each of these processes.

Each process is responsible for propagating a subset of the points it knows. Before communication, exactly one process should be responsible for propagating each point. The intersecting processes for each point are determined - by the responsible process - with p4est_search_partition. Points are then communicated to the relevant processes. Points known to a process before communication that do not intersect its domain are forgotten. The algorithm ensures that after communication exactly one process is responsible for the propagation of each point. This is the process with the lowest rank among processes intersecting the point.

I have rewritten my portion of the GMT example to use this abstracted version. This rewrite breaks the other parts of the example, due to typing changes, such as the intersection callback having a different type. The rewrite can be found here: https://github.com/ljcarlin/p4est/tree/feature-gmt

[ljcarlin]

@cburstedde I added more documentation and a \file so that p4est_communication.h is parsed by doxygen. If it was intentional not to have that then I can remove it again.

I also noticed that doxygen mistakes \ref p4est_transfer_search as referring to the structure p4est_transfer_search behind the type p4est_transfer_search_t rather than the function p4est_transfer_search. Basically when you typedef and define a struct at the same time then doxygen interprets the comment as referring to the struct rather than the type. I think this happens elsewhere in p4est too.

[cburstedde]

@cburstedde I added more documentation and a `\file` so that `p4est_communication.h` is parsed by doxygen. If it was intentional not to have that then I can remove it again.

Cool; thanks for the effort!

I also noticed that doxygen mistakes `\ref p4est_transfer_search` as referring to the structure `p4est_transfer_search` behind the type `p4est_transfer_search_t` rather than the function `p4est_transfer_search`. Basically when you typedef and define a struct at the same time then doxygen interprets the comment as referring to the struct rather than the type. I think this happens elsewhere in p4est too.

This is unfortunate; thanks for figuring this out. So would we want to rename every struct behind a typedef by adding _s? That would affect quite a bit of the code.

[ljcarlin]

This is unfortunate; thanks for figuring this out. So would we want to rename every struct behind a typedef by adding _s? That would affect quite a bit of the code.

Yes, it seems quite unfortunate.

Another approach is to declare the struct first (without documentation) and then document the _t typedef. I think this makes the code a bit less readable though, and it requires just as much code change.

[cburstedde]

@cburstedde I added more documentation and a \file so that p4est_communication.h is parsed by doxygen. If it was intentional not to have that then I can remove it again.

This is very helpful; thanks! We're behind on some documentation.

I also noticed that doxygen mistakes \ref p4est_transfer_search as referring to the structure p4est_transfer_search behind the type p4est_transfer_search_t rather than the function p4est_transfer_search. Basically when you typedef and define a struct at the same time then doxygen interprets the comment as referring to the struct rather than the type. I think this happens elsewhere in p4est too.

Outside of this PR, which other struct names may be in need of a _s suffix?

[cburstedde]

I'm now thinking about the unowned points. By definition they are part of the propagated portion of the points array (?). Should we indicate which of the propagated are unowned? We may put these into a contiguous range right in the beginning and add a num_unowned member?

[ljcarlin]

Outside of this PR, which other struct names may be in need of a _s suffix?

I noticed you fixed this just by changing the name of the struct. This is probably a better solution. I don't think the issue actually appears very frequently, I had a quick look and couldn't see it anywhere else.

[ljcarlin]

I'm now thinking about the unowned points. By definition they are part of the propagated portion of the points array (?). Should we indicate which of the propagated are unowned? We may put these into a contiguous range right in the beginning and add a num_unowned member?

Sure, that sounds reasonable. As is, a search based refinement would be refining with the irrelevant unowned points, which is a bit silly.

[cburstedde]

> Outside of this PR, which other struct names may be in need of a _s suffix? I noticed you fixed this just by changing the name of the struct. This is probably a better solution. I don't think the issue actually appears very frequently, I had a quick look and couldn't see it anywhere else.

Thanks!

[cburstedde]

> I'm now thinking about the unowned points. By definition they are part of the propagated portion of the points array (?). Should we indicate which of the propagated are unowned? We may put these into a contiguous range right in the beginning and add a num_unowned member? Sure, that sounds reasonable. As is, a search based refinement would be refining with the irrelevant unowned points, which is a bit silly.

Cool, thanks for the update! It seems with this, the unowned feature is now fully functional as seen from the outside (?). I have one minor suggestion: We might define a dummy p4est to always have valid mpicomm, mpisize, and mpirank members in addition to the user_pointer possibly being set. The rest of the bytes of the p4est struct shall be initialized to zero. This would allow us to cut a few communicator-related arguments passed to static functions. Similarly, we may not pass point_size in that one instance where the internal structure is passed already. About the error handling: with 'errsend = errsend || ...' the compiler omits the execution of the second expression if the first is already true. It seems that this is intentional, just checking that per-rank error conditions don't hang the parallel execution of the algorithm. And, lastly, may I ask to run `./p4estindent` on the updated files? Beware however that some 'features' of that script require manual override, such as obvious nonsense and function arguments: use `(type_t *var)`, not `* var`.

[cburstedde]

What we might also do is adding a third variable to the context, num_total. This is redundant but maybe a nice feature for consistency.

[cburstedde]

Ok, let's not to the dummy p4est since your code aligns perfectly with the search_gf* routines.

Just remaining to look into the errsend question and indenting the .c file (I've done it for the .h).