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

Reduce the DistributedBase interface #1606

Merged
merged 5 commits into from
May 15, 2024
Merged

Conversation

MarcelKoch
Copy link
Member

@MarcelKoch MarcelKoch commented May 7, 2024

This PR reduces the interface changes to the DistributedBase interface. It removes the virtual function get_local_size. The only place where this was used was in the multigrid solver. There it has been replaced by using the run dispatch with every possible distributed Matrix type.

It also makes run easier to use by automatically deducing the const qualifier.

Note: although this is based on #1544, it can be rebased on develop and merged before.

@MarcelKoch MarcelKoch self-assigned this May 7, 2024
@MarcelKoch MarcelKoch added this to the Ginkgo 1.8.0 milestone May 7, 2024
@ginkgo-bot ginkgo-bot added mod:core This is related to the core module. type:solver This is related to the solvers type:matrix-format This is related to the Matrix formats labels May 7, 2024
@MarcelKoch MarcelKoch force-pushed the read-distributed-with-index-map branch from 613231d to daf690d Compare May 7, 2024 12:51
@MarcelKoch MarcelKoch force-pushed the reduce-dist-base-interface branch 2 times, most recently from ca9cb11 to bfeca24 Compare May 7, 2024 14:25
@MarcelKoch MarcelKoch force-pushed the read-distributed-with-index-map branch from 40f74db to fda0fc9 Compare May 7, 2024 14:26
@MarcelKoch MarcelKoch force-pushed the read-distributed-with-index-map branch from fda0fc9 to 51ff6f9 Compare May 8, 2024 07:03
@MarcelKoch MarcelKoch force-pushed the reduce-dist-base-interface branch 2 times, most recently from 74f81ff to afb7bcf Compare May 8, 2024 09:17
@MarcelKoch MarcelKoch force-pushed the read-distributed-with-index-map branch from 51ff6f9 to 37aef87 Compare May 8, 2024 09:17
@MarcelKoch MarcelKoch force-pushed the read-distributed-with-index-map branch from f2cd744 to 2072219 Compare May 8, 2024 18:53
@MarcelKoch MarcelKoch force-pushed the reduce-dist-base-interface branch 2 times, most recently from 0c725a9 to e6543ec Compare May 9, 2024 07:55
@MarcelKoch MarcelKoch force-pushed the read-distributed-with-index-map branch from 20e3d96 to 2812123 Compare May 9, 2024 08:56
@MarcelKoch MarcelKoch added the 1:ST:ready-for-review This PR is ready for review label May 10, 2024
@MarcelKoch MarcelKoch requested review from thoasm and yhmtsai May 10, 2024 09:23
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. I have added the shared_ptr version previously, so I hold my approval until the shared_ptr still works here. It should work seamlessly with these changes though.

*
* @return the result of f invoked with obj cast to the first matching type
*/
template <template <class> class K, typename... Types, typename T,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
template <template <class> class K, typename... Types, typename T,
template <template <class> class Base, typename... Types, typename T,

also the documentation needs to be adjusted. I have changed some documentations here, you may need to rebase and keep them similar.
With that, I think the following function can be deleted, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Which following function do you mean? This at l.221 or at l.252?

@MarcelKoch MarcelKoch force-pushed the reduce-dist-base-interface branch 2 times, most recently from 1ae79b6 to d35432b Compare May 13, 2024 09:07
@MarcelKoch MarcelKoch changed the base branch from read-distributed-with-index-map to develop May 13, 2024 09:08
@MarcelKoch
Copy link
Member Author

@yhmtsai I've unified the shared_ptr implementation with the previous smart pointer implementation.

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. but unique_ptr part is unneccessary now.

* @tparam Func the function type that is invoked if the object can be
* converted to std::shared_ptr<K>
* converted to pointer of const Base<K>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* converted to pointer of const Base<K>
* converted to pointer of Base<K>

The const depends on the given type now, right?

Comment on lines 240 to 241
* @tparam T the type of input object waiting converted, has to be either
* unique_ptr or shared_ptr
Copy link
Member

Choose a reason for hiding this comment

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

It can only be shared_ptr because dynamic_pointer_cast only allow shared_ptr and the ownership issue.

core/base/dispatch_helper.hpp Outdated Show resolved Hide resolved
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.

I think the pgm headers should be left in the pgm related files. Otherwise LGTM

omp/multigrid/pgm_kernels.cpp Show resolved Hide resolved
reference/multigrid/pgm_kernels.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 MarcelKoch merged commit 368fada into develop May 15, 2024
10 of 15 checks passed
@MarcelKoch MarcelKoch deleted the reduce-dist-base-interface branch May 15, 2024 10:02
Copy link

sonarcloud bot commented May 16, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-to-merge This PR is ready to merge. mod:core This is related to the core module. type:matrix-format This is related to the Matrix formats type:solver This is related to the solvers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants