Feat halo #197
Feat halo #197
Conversation
…nd reuses values) fixed boundary/halo views and size calculation replaced std::bind with lambda
…nd reuses values) fixed boundary/halo views and size calculation replaced std::bind with lambda
… environment in future
…halo Conflicts: dash/include/dash/Cartesian.h
|
Unit tests are currently missing as well. There is a TODO in According to my stencil example, I assume that something with the boundary region is not correct. Unfortunately I do not know, whether the bug is in my code or in DASH. ex.11.halo-stencil
ex.11.simple.stencil
|
|
Great! Will review all changes back at the hotel and put my comments here. |
| namespace dash { | ||
|
|
||
| template<typename MatrixT, typename HaloSpecT> | ||
| class HaloMatrix |
There was a problem hiding this comment.
The design of dash::HaloMatrix violates a lot of concepts and the container definition.
For example, ibegin, iend and bbegin, bend should be views, like dash::HaloMatrix.boundary.begin|.end.
Methods updateHalo, waitHaloAsync etc. are obviously not container concerns, but algorithms.
We would have to re-implement those for every container that supports boundary regions.
I'm okay with putting this implementation in examples, but we can't publish it as a container type.
There was a problem hiding this comment.
Furthermore it's cumbersome to use it, as no access to the underlying container is provided. Hence the halo has to be created on each iteration (see here).
For me, a halo is a prime example for the decorator pattern, because the halo adds features to a container. It's not really an algorithm. However halos should support all containers with block views.
There was a problem hiding this comment.
@fmoessbauer Yes, that was my intention with dash::HaloBlock exactly. It's just a view on a regular NArray block. dash::HaloBlock then again represents a sequence container.
See:
- HaloBlock:
https://github.com/dash-project/dash/blob/development/dash/include/dash/Halo.h#L1274 - BlockBoundaryView:
https://github.com/dash-project/dash/blob/development/dash/include/dash/Halo.h#L1064 - Unit tests of
GlobStencilIter,HaloBlock:
https://github.com/dash-project/dash/blob/development/dash/test/GlobStencilIterTest.cc#L308
The unit tests demonstrate the usage of halo views like:
auto boundary_begin = halo_block.boundary().begin();
auto boundary_end = halo_block.boundary().end();
auto halo_begin = halo_block.halo().begin();
auto halo_end = halo_block.halo().end();
// north halo region:
auto h_region_n = halo_block.halo_region({ -1, 0 });
auto h_region_n_begin = h_region_n.begin();
auto h_region_n_end = h_region_n.end();There was a problem hiding this comment.
Hrrrmmm, I had a variant of dash::Matrix where the block view type could be specified as a template parameter. Didn't migrate the branch with the implementation to github, apparently.
There was a problem hiding this comment.
I prefer moving it to dash/experimental, as already two examples are using the halo stuff. dash/experimental won't be included in libdash.h so whenever a user dares to use it, he has to include dash/experimental/... explicitly.
Furthermore moving it to examples will give name clashes in the future.
There was a problem hiding this comment.
Eh, "experimental" is for concept proposals actually, but it's okay, will refactor it some time soon.
|
IMO it can be merged. |
|
The original implementations of |
Merge halo implementation of @dhinf in development (#195). This feature is especially needed in #196 and the astro tutorial.
Please review carefully and also check if the tests run on a "real" cluster. Not just on online CIs, as there at max 3 units are spin up.