Skip to content
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

[compute/cker] Introduce the ShapeIterator #14311

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tomdol
Copy link
Contributor

@tomdol tomdol commented Nov 15, 2024

This commit introduces a utility that effectively makes the Shape objects iterable. It's an iterator class which points to the individual dimensions in the shape and allows the interoperability of the Shape class and STL algorithms as well as range-based for loops. The iterator fulfills the requirements of a bidirectional iterator.

In addition this commit contains one extra utility which allows the Shape objects conversion to std::string by contatenating them with a comma.

ONE-DCO-1.0-Signed-off-by: Tomasz Dolbniak t.dolbniak@partner.samsung.com

This commit introduces an utility that effectively makes the Shape objects iterable. It's an iterator class which points to the individual dimensions in the shape and allows the interoperability of the Shape class and STL algorithms as well as range-based for loops. The iterator fulfills the requirements of a bidirectional iterator.

In addition this commit contains one extra utility which allows the Shape objects conversion to std::string by contatenating them with a comma.

ONE-DCO-1.0-Signed-off-by: Tomasz Dolbniak <t.dolbniak@partner.samsung.com>
@tomdol tomdol requested a review from a team November 15, 2024 12:46
@zetwhite
Copy link
Contributor

@nnfw-bot test onert-x64-release

@tomdol
Copy link
Contributor Author

tomdol commented Nov 20, 2024

@zetwhite could you please review or suggest particular reviewers for this PR?

@hseok-oh
Copy link
Contributor

@tomdol

  1. What is purpose of these utilities? Where will be it used?
  2. Why neg_ShapeIterator_empty_shape have negative test naming?

@tomdol
Copy link
Contributor Author

tomdol commented Nov 20, 2024

@hseok-oh the purpose is to extend the ability to iterate over the Shape dimensions. With the Shape class' API it's only possible to use it with traditional indexed loops and the Shape::Dims(int) method to access individual dimensions. The shape iterator adds the ability to iterate over a shape with a range based for loop:

Shape shape{1,2,3,4};
for (auto&& dimension : shape) {}

or with algorithms expecting a pair of iterators denoting a range of dimensions:

Shape shape{1,2,3,4};
const auto first = begin(shape);
const auto last = end(shape);
std::vector<int> filtered_dims;
// copy the dimensions that are different than 1
std::copy_if(first, last, begin(filtered_dims), [](int dimension) { return dimension != 1; });
// or:
// accumulate all dimensions to get the numer of elements in a tensor
// or:
// get the iterator to the first dimension, skip over the batch and channels and process the remaining dimensions using STL

It's difficult to list all possible use cases but the main thing I was missing was the ability to use STL algorithms when traversing a Shape object.

Regarding the test naming, I know there's a requirement for the number of negative tests but couldn't really come up with a good negative test for this utility. Should that be changed in any way?

@zetwhite zetwhite requested a review from a team November 21, 2024 01:52
@hseok-oh
Copy link
Contributor

hseok-oh commented Nov 21, 2024

The Shape class is comes from TFLite's RuntimeShape. In fact, most of cker's implementation is porting of TFLite. We may need to update Shape class for our needs.

Anyway, this PR looks good.

Co-authored-by: SeungHui Youn <61981457+zetwhite@users.noreply.github.com>
Copy link
Contributor

@hseok-oh hseok-oh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants