-
Notifications
You must be signed in to change notification settings - Fork 88
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 segmented array type #1545
Add segmented array type #1545
Conversation
Do you actually need these individual sub-ranges to be materialized as |
At least for CPU kernels the arrays are quite handy. But you're right, something like |
I've had it in my mind to provide a nice abstraction over these segmented arrays (including range-for support) for a while already, if you want I can resurrect my prototype code |
Sure, if it's not too much work, we could use that. I can also take that over if you want. |
da82c05
to
624fddb
Compare
Quality Gate passedIssues Measures |
I don't think that is a good abstraction, since the size per dimension isn't constant. |
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 is a useful construct, I would try to make it more generic in some contexts (index type) and more restricted in others (host-accessibility). I would imagine a device adapter based on what I implemented as segmented_range
in #1582 might be useful here.
Also we would need some tests and documentation before we can merge this
* \tparam T value type stored in the arrays | ||
*/ | ||
template <typename T> | ||
struct array { |
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 I would call this a segmented array, since the corresponding operations are referred to a segmented sort/reduction/... in papers and the cub library. That would also avoid the need for a separate namespace
for (size_type i = 0; i < sizes.size(); ++i) { | ||
elems_[i] = make_array_view( | ||
exec, sizes[i], buffer_.get_data() + offsets_.get_data()[i]); | ||
} |
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 design might encourage poor performance - it invites using a kernel invocation for each MPI rank, which might scale poorly. Instead I would try to do this with a single kernel for all ranks, and a corresponding device view.
|
||
private: | ||
gko::array<T> buffer_; | ||
gko::array<size_type> offsets_; |
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.
Considering #430, I would prefer if we used int64 or even a generic integer type.
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 agree with Tobias that calling it segmented_array
would be better. Otherwise, I dont have any other concerns.
624fddb
to
6d7858e
Compare
6d7858e
to
2a0ba2b
Compare
2a0ba2b
to
98527d4
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.
LGTM mostly, does it need to be part of the public interface, or can we put it in detail
first? I believe it might change significantly with segmented ranges.
98527d4
to
e7ef84d
Compare
@upsj I don't think this will change with the segmented ranges at all. The ranges are only relevant on the backend side. The |
e7ef84d
to
5bf7f9c
Compare
Maybe also update the PR name ? |
5bf7f9c
to
c3b7bcc
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.
Mostly looks good. Just one question regarding the _const_x
naming for const getters.
c3b7bcc
to
6cc3634
Compare
e20d10c
to
cf07464
Compare
template <typename T> | ||
T* segmented_array<T>::get_flat_data() | ||
{ | ||
return buffer_.get_data(); |
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.
You can move this directly within the class, right ?
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 can actually move them into the .cpp. I did that, but I won't wait for the full CI pipeline to merge it.
Co-authored-by: Pratik Nayak <pratik.nayak@kit.edu>
cf07464
to
dfcf799
Compare
This PR adds a class to store a segmented array. A flat ginkgo array is partitioned into multiple segments by an additional index offset array. The segment
i
starts within the flat buffer at indexoffsets[i]
, and ends (exclusively) at indexoffsets[i + 1]
. The class only provides access to the flat buffer and the offsets.Used in: #1543
PR Stack:
Old Description
This PR adds a type
collection::array
which is astd::vector
like class that stores multiple arrays. These arrays are all subviews of a shared buffer.I think technically this could be considered as a
batch::array
, but I'm not sure if we want to go that way. As abatch::array
this would interleave the dependencies between the batched and non-batched stuff, and I'm not sure if that is a good idea.