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

Add specific communicator for neighborhood communication (Again) #1780

Merged
merged 13 commits into from
Feb 26, 2025

Conversation

MarcelKoch
Copy link
Member

This is just #1588, which was marked as merged, because I reordered some commits, and GH interpreted this as the neighborhood-communicator branch being merged.
For comments and details, see the previous PR.

@MarcelKoch MarcelKoch added the 1:ST:ready-for-review This PR is ready for review label Feb 14, 2025
@MarcelKoch MarcelKoch added this to the Ginkgo 1.10.0 milestone Feb 14, 2025
@MarcelKoch MarcelKoch self-assigned this Feb 14, 2025
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. mod:core This is related to the core module. labels Feb 14, 2025
@MarcelKoch MarcelKoch mentioned this pull request Feb 14, 2025
4 tasks
@@ -0,0 +1,37 @@
// SPDX-FileCopyrightText: 2024 - 2025 The Ginkgo authors
Copy link
Member

Choose a reason for hiding this comment

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

I think the range of years is somehow incorrect ? I think it should 2017 - 2025 ? Or is it different for each file ?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO it's not important that every file has the copyright 2017-2025. This file didn't exist 2017, so putting a copyright for that year into it seems a bit pointless. Also this is just what the reuse tool does, so we should stick to it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that is fine, but neighborhood_communicator.cpp also didnt exist in 2017. So, I would prefer that we be uniform.

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH, I will not change the copyright header that is added by our tooling. I think the reason for the difference is that the file with 2017-2025 was created when we still had our custom format_header.sh, which always put in the 2017. New files created from now on will not have the 2017 anymore.

@@ -0,0 +1,273 @@
// SPDX-FileCopyrightText: 2017 - 2025 The Ginkgo authors
Copy link
Member

Choose a reason for hiding this comment

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

Here it is 2017 - 2025.

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.

need to apply formatting on ginkgo.hpp again due to the order of pre-commit order.

@pratikvn pratikvn self-requested a review February 17, 2025 07:55
@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 Feb 17, 2025
@MarcelKoch MarcelKoch mentioned this pull request Feb 18, 2025
2 tasks
@MarcelKoch MarcelKoch force-pushed the neighborhood-communicator branch 2 times, most recently from a1b5990 to 37f5bba Compare February 18, 2025 15:46
MarcelKoch and others added 11 commits February 25, 2025 07:35
Signed-off-by: Marcel Koch <[email protected]>
Co-authored-by: Pratik Nayak <[email protected]>
Signed-off-by: Marcel Koch <[email protected]>
- fix include guards
- update docs
- implement copy/move constructors/assignment with tests
- add equality test for collective communicators (needed for testing)
- always enable neighborhood comm, just throw if openmpi is too old
- define moved-from state as MPI_COMM_NULL + empty sizes/offsets
- remove unnecessary namespace
- make virtual function protected

Co-authored-by: Pratik Nayak <[email protected]>
Co-authored-by: Tobias Ribizel <[email protected]>
Signed-off-by: Marcel Koch <[email protected]>
MarcelKoch and others added 2 commits February 25, 2025 07:35
- documentation
- formatting
- refactor test
- fix docs
- merge tests

Co-authored-by: Yu-Hsiang M. Tsai <[email protected]>
Signed-off-by: Marcel Koch <[email protected]>
@MarcelKoch MarcelKoch force-pushed the neighborhood-communicator branch from 37f5bba to d7d3d40 Compare February 25, 2025 07:35
Copy link

codecov bot commented Feb 25, 2025

Codecov Report

Attention: Patch coverage is 58.80399% with 124 lines in your changes missing coverage. Please review.

Project coverage is 89.17%. Comparing base (ac0ffbd) to head (d7d3d40).
Report is 14 commits behind head on develop.

Files with missing lines Patch % Lines
core/distributed/neighborhood_communicator.cpp 0.00% 104 Missing ⚠️
include/ginkgo/core/base/mpi.hpp 50.00% 14 Missing ⚠️
core/distributed/dense_communicator.cpp 90.90% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1780      +/-   ##
===========================================
- Coverage    89.31%   89.17%   -0.15%     
===========================================
  Files          811      817       +6     
  Lines        67514    67816     +302     
===========================================
+ Hits         60301    60474     +173     
- Misses        7213     7342     +129     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MarcelKoch MarcelKoch merged commit ebb4725 into develop Feb 26, 2025
13 of 14 checks passed
@MarcelKoch MarcelKoch deleted the neighborhood-communicator branch February 26, 2025 12:16
@MarcelKoch
Copy link
Member Author

A note on the reported test coverage: the neighborhood communicator is completely missing, because the CI image uses an old open mpi version, for which the neighborhood implementation is deactivated.

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. reg:build This is related to the build system. reg:testing This is related to testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants