-
Notifications
You must be signed in to change notification settings - Fork 15
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
WIP - Introduces bounds.h along with tests. #83
base: master
Are you sure you want to change the base?
Conversation
@@ -982,6 +982,10 @@ NDARRAY_HOST_DEVICE shape<Dims...> make_shape_from_tuple(const std::tuple<Dims.. | |||
template <size_t Rank> | |||
using index_of_rank = internal::tuple_of_n<index_t, Rank>; | |||
|
|||
// ELEPHANT |
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.
Don't submit this yet. Introduced to try constructing a shape
from a bounds
.
return bounds<Intervals...>(intervals); | ||
} | ||
|
||
template <class... Intervals> |
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 cribbed from shape
. I can add more comments once we settle on a design.
NDARRAY_HOST_DEVICE const intervals_type& intervals() const { return intervals_; } | ||
|
||
/** Makes a tuple of dims out of each interval in this bounds. */ | ||
NDARRAY_HOST_DEVICE auto dims() const { return internal::dims(intervals(), interval_indices()); } |
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.
bounds
is just a shape
without most of the features. It has only min and extents, either of which can be static.
dims()
is the only fun function, which converts to a tuple<dim<...>, dim, dim, ...>
appropriately, which can then be used to construct a shape
.
"shuffle.cpp", | ||
"sort.cpp", | ||
"split.cpp", | ||
"bounds.cpp", |
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.
Revert once ready.
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 general the high level direction is good, we should have a way to represent shapes that don't have strides. I've been struggling with this for a while (maybe #24 is related too).
One reason I've hesitated to do something is I was trying to find a way forward without duplicating so much of shape. This isn't just motivated by code deduplication, I also want to minimize the number of types/stuff in the API, especially having two very similar types is unfortunate.
What if we just have a helper that drops all strides from shape? And maybe also try to make shapes easier to work with if you don't care about strides (if that's even an issue now)? array
/array_ref
could have a helper function bounds()
that just returns the shape with all strides removed.
On the other hand, we already have interval
and dim
. So maybe it's natural we have bounds
and shape
. But then again, I added interval
late, only after "giving up" on the above arguments (and interval
/dim
are much smaller types).
Anyways, that's just some rambling thoughts on the issue, I'll probably remember more later too.
I agree that we don't want to introduce too many types and therefore concepts to the client. I think the set (heh, category?) of concepts here is unfortunately a commutative square because we have:
So the two axes are "singleton vs list" and "with or without stride". And I think both are useful.
|
No description provided.