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.

Sign up for GitHub

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

Jump to bottom

Distributed Index Map #1543

Merged
merged 7 commits into from
May 13, 2024
Merged

Distributed Index Map #1543

merged 7 commits into from
May 13, 2024

Conversation

MarcelKoch
Copy link
Member

@MarcelKoch MarcelKoch commented Feb 14, 2024 •
edited
Loading

This PR adds an index_map to be used in our distributed components. This PR only adds the core and reference kernels. device kernels are added in #1579

I will add another PR that shows how this will be used.

PR Stack:

@MarcelKoch MarcelKoch self-assigned this Feb 14, 2024
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. mod:all This touches all Ginkgo modules. labels Feb 14, 2024
This was referenced Feb 14, 2024
Merged
Merged
@MarcelKoch MarcelKoch force-pushed the index-map branch 2 times, most recently from 6a173a4 to a7d1dab Compare March 25, 2024 10:31
This was referenced Mar 25, 2024
Merged
Closed
Closed
Closed
@pratikvn pratikvn self-requested a review March 25, 2024 10:53
@MarcelKoch MarcelKoch added this to the Ginkgo 1.8.0 milestone Mar 25, 2024
pratikvn
pratikvn requested changes Mar 30, 2024
View reviewed changes
Copy link
Member

@pratikvn pratikvn left a comment

Choose a reason for hiding this comment

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

A couple of minor things, but mostly looks okay to me.

core/distributed/index_map.cpp Outdated Show resolved Hide resolved
core/distributed/index_map_kernels.hpp Outdated Show resolved Hide resolved
core/distributed/index_map_kernels.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/distributed/index_map.hpp Show resolved Hide resolved
include/ginkgo/core/distributed/index_map.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/distributed/index_map.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/distributed/index_map.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/distributed/index_map.hpp Outdated Show resolved Hide resolved
reference/distributed/index_map_kernels.cpp Outdated Show resolved Hide resolved
reference/test/distributed/index_map_kernels.cpp Outdated Show resolved Hide resolved
@MarcelKoch MarcelKoch marked this pull request as ready for review April 2, 2024 07:37
pratikvn
pratikvn approved these changes Apr 2, 2024
View reviewed changes
Copy link
Member

@pratikvn pratikvn left a comment

Choose a reason for hiding this comment

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

LGTM!

reference/distributed/index_map_kernels.cpp Show resolved Hide resolved
upsj
upsj reviewed Apr 3, 2024
View reviewed changes
Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

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

First look at the interface

include/ginkgo/core/distributed/index_map.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/distributed/index_map.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/distributed/index_map.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/distributed/index_map.hpp Outdated Show resolved Hide resolved
@MarcelKoch MarcelKoch force-pushed the index-map branch 2 times, most recently from 1f133bf to 61f5b3d Compare April 4, 2024 11:00
This was referenced Apr 30, 2024
Open
Open
@MarcelKoch MarcelKoch force-pushed the index-map branch 3 times, most recently from 17c52a5 to 621ef1a Compare May 2, 2024 10:04
@MarcelKoch MarcelKoch force-pushed the array-collection branch 2 times, most recently from e20d10c to cf07464 Compare May 6, 2024 16:38
@MarcelKoch MarcelKoch force-pushed the index-map branch 2 times, most recently from 8cb01b4 to f7fa971 Compare May 7, 2024 12:13
@MarcelKoch MarcelKoch requested a review from upsj May 7, 2024 12:49
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 8, 2024

Quality Gate Passed Quality Gate passed

Issues
25 New issues
0 Accepted issues

Measures
0 Security Hotspots
66.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Base automatically changed from array-collection to develop May 9, 2024 07:38
@MarcelKoch MarcelKoch force-pushed the index-map branch 2 times, most recently from 4e0efb4 to 92a2e8d Compare May 9, 2024 08:56
@MarcelKoch MarcelKoch dismissed pratikvn’s stale review May 9, 2024 08:57

added device view of segmented arrays

@MarcelKoch MarcelKoch requested a review from pratikvn May 9, 2024 08:57
upsj
upsj approved these changes May 13, 2024
View reviewed changes
Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

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

LGTM! Only minor point is gko::vector usage to track memory allocations

core/distributed/index_map.cpp
{
if (this != &other) {
partition_ = std::move(other.partition_);
rank_ = other.rank_;
Copy link
Member

Choose a reason for hiding this comment

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

nit: should the moved-from state preserve the original rank?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that is sensible. We don't really have an invalid rank value that I could put here.

core/distributed/index_map_kernels.hpp Outdated Show resolved Hide resolved
core/test/distributed/index_map.cpp Show resolved Hide resolved
include/ginkgo/core/distributed/index_map.hpp Outdated Show resolved Hide resolved
reference/distributed/index_map_kernels.cpp Show resolved Hide resolved
reference/distributed/index_map_kernels.cpp Outdated Show resolved Hide resolved
pratikvn
pratikvn approved these changes May 13, 2024
View reviewed changes
Copy link
Member

@pratikvn pratikvn left a comment

Choose a reason for hiding this comment

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

LGTM!

core/distributed/index_map.cpp Show resolved Hide resolved
@MarcelKoch MarcelKoch added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels May 13, 2024
MarcelKoch and others added 7 commits May 13, 2024 09:43
@MarcelKoch
extract common partition helpers
ee96302
@MarcelKoch
handle zero parts in partition::build_from_global_size
37e1f6a
@MarcelKoch
add device_segmented_array for kernel use
0e6107f
@MarcelKoch
adds distributed index map
122999f
@MarcelKoch
adds distributed index map core & reference
7db69d2
@MarcelKoch
adds distributed index map core + reference tests
a5ae003
@MarcelKoch @pratikvn @upsj
review updates:
4dc3658 
- documentation
- renaming
- use gko::vector
- test move-out state

Co-authored-by: Pratik Nayak <pratik.nayak@kit.edu>
Co-authored-by: Tobias Ribizel <mail@ribizel.de>
Co-authored-by: Gregor Olenik <gregor.olenik@kit.edu>
@MarcelKoch MarcelKoch merged commit 5e584b5 into develop May 13, 2024
10 of 15 checks passed
@MarcelKoch MarcelKoch deleted the index-map branch May 13, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@upsj upsj upsj approved these changes

@pratikvn pratikvn pratikvn approved these changes

Assignees

@MarcelKoch MarcelKoch

Labels
1:ST:ready-to-merge This PR is ready to merge. 1:ST:run-full-test mod:all This touches all Ginkgo modules. reg:build This is related to the build system. reg:testing This is related to testing.
Projects
None yet
Milestone
Ginkgo 1.8.0
Development

Successfully merging this pull request may close these issues.
4 participants
@MarcelKoch @upsj @pratikvn @ginkgo-bot