Simple segmented ranges #1601
Simple segmented ranges #1601
Conversation
ginkgo-bot
added
reg:build
upsj
force-pushed
the
simple_segmented_ranges
branch
from
bba6eba
to
4b9a6cd
Compare
|
First up, the zip iterator also need to be made constexpr for this to work on the device. |
There was a problem hiding this comment.
this looks a lot cleaner than the old PR, thanks for that. I have mostly smaller remarks, the biggest issue is the zip_iterator which is not available on the device. Maybe it would be possible to use different types for the zip_iterator depending on the backend.
upsj
force-pushed
the
simple_segmented_ranges
branch
from
4b9a6cd
to
03a95bf
Compare
upsj
force-pushed
the
simple_segmented_ranges
branch
from
6b2393e
to
86218ce
Compare
upsj
force-pushed
the
simple_segmented_ranges
branch
from
86218ce
to
e94ad77
Compare
upsj
force-pushed
the
device_zip_iterator
branch
2 times, most recently
from
b22f9fe
to
76dd812
Compare
upsj
force-pushed
the
simple_segmented_ranges
branch
from
758f690
to
cddc108
Compare
| index_type index; | ||
| segment_type segment; |
There was a problem hiding this comment.
I do find the naming a bit confusing. I think index is the index of the segment, and not an index of the range. And segment is not the index of the segment, but a full range by itself.
There was a problem hiding this comment.
The "entries" of a segmented range are segments, and enumerating them you get a segment index and segment value back. Do you have a suggestion how to make this more clear through naming?
| * @tparam ValueIterator the iterator type pointing to the values. | ||
| */ | ||
| template <typename IndexType, typename ValueIterator> | ||
| class segmented_value_range { |
There was a problem hiding this comment.
I'm wondering a bit how this will work out with the device_segmented_array and device implementations of our classes in general.
In principle, device_segmented_array could be replaced directly by this. One thing I would not like about that is that the device_segmented_array basically consists only of data members, and in general I would prefer our device implementations to not have member functions. (My reason for that is that I want to generalize the device implementations so that they can be directly exposed to kokkos or similar.)
So right now, I would imagine conversion functions and having some redundancy, instead of replacing device_segmented_array.
There was a problem hiding this comment.
I think that is a heavy restriction. Is there a reason why having no member functions is necessary to expose these directly to kokkos ?
There was a problem hiding this comment.
It's not necessary. But I would prefer to not provide methods, as I'm not sure if they would also be available in Kokkos code. I guess they would be, since this can run on the GPU, but I would still be a bit hesitant.
| * @tparam ValueIterator the iterator type pointing to the values. | ||
| */ | ||
| template <typename IndexType, typename ValueIterator> | ||
| class segmented_value_range { |
There was a problem hiding this comment.
I think that is a heavy restriction. Is there a reason why having no member functions is necessary to expose these directly to kokkos ?
upsj
force-pushed
the
simple_segmented_ranges
branch
from
bdeddf6
to
f61eff4
Compare
upsj
added
1:ST:ready-to-merge
- fix name hiding in classes - add tests for ptrs constructors Co-authored-by: Marcel Koch <marcel.koch@kit.edu>
upsj
force-pushed
the
simple_segmented_ranges
branch
from
a20de3e
to
9fbbbba
Compare
This removes all stride support from #1582, which removes most of the heavy
iteratorfunctionality added in the other PR.It relies on
zip_iteratorto provide both enumeratingfor (auto [nonzero_index, column, value] : matrix.enumerated()[row])and non-enumeratingfor (auto [column, value] : matrix[row])iteration through a matrix, and uses pairs of pointers to represent ranges of column indices or values.The two commits add the baseline first and C++17 structured binding support second.
TODO
zip_iteratorto be used on the device #1604