Skip to content

Add transform_iterator #1836

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Add transform_iterator #1836

wants to merge 2 commits into from

Conversation

upsj
Copy link
Member

@upsj upsj commented Apr 27, 2025

This adds a transform_iterator that can be used with standard library algorithms and other functions like the ones from #1820

@upsj upsj added the 1:ST:ready-for-review This PR is ready for review label Apr 27, 2025
@upsj upsj requested review from MarcelKoch and a team April 27, 2025 10:44
@upsj upsj self-assigned this Apr 27, 2025
@ginkgo-bot ginkgo-bot added reg:testing This is related to testing. mod:core This is related to the core module. labels Apr 27, 2025
@upsj upsj force-pushed the transform_iterator branch from 360872b to 32068ed Compare April 27, 2025 10:48
auto test_iter = gko::detail::make_transform_iterator<TypeParam*>(
nullptr, [](TypeParam v) { return v; });

ASSERT_NO_THROW((void)std::find(test_iter, test_iter, TypeParam{}));
Copy link
Member

Choose a reason for hiding this comment

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

does it check anything? std::find does not do anything because first and end are the same.
also what is (void) for?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is just a copy of the same test for permute_iterator. I needed to use a read-only operation, so find was the first thing that came to mind. The (void) is there to silence the [[nodiscard]] warning.

@upsj upsj force-pushed the transform_iterator branch from 32068ed to 34f6a8c Compare April 30, 2025 14:52
@upsj upsj changed the base branch from develop to bitvector April 30, 2025 14:52
@upsj upsj force-pushed the transform_iterator branch from 34f6a8c to a94c5a8 Compare April 30, 2025 15:45
Base automatically changed from bitvector to develop April 30, 2025 18:49
Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

LGTM from the implementation, but I am not familiar with the iterator implementation.
If someone can review it to check whether it match the std iterator requirement or you can give the reference. I only check the iterator_traits and the random access iterator requirement.
I assume it will just give a compilation error when the Iterator is not random accessable because the following member functions uses the iterator as random access iterator.

TYPED_TEST(TransformIterator, IncreasingIterator)
{
std::vector<TypeParam> vec{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
auto transform = double_transform{};
Copy link
Member

Choose a reason for hiding this comment

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

The following two tests do not check the value after transformation, so I think use scale_transofrm should be good enough

@MarcelKoch MarcelKoch added this to the Ginkgo 1.10.0 milestone May 13, 2025
@MarcelKoch MarcelKoch removed this from the Ginkgo 1.10.0 milestone May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-for-review This PR is ready for review mod:core This is related to the core module. reg:testing This is related to testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants