Skip to content

[Algs] add are_all_rank_true collective#1248

Merged
tdavidcl merged 7 commits intoShamrock-code:mainfrom
tdavidcl:patch-2025-09-04-08-55
Sep 4, 2025
Merged

[Algs] add are_all_rank_true collective#1248
tdavidcl merged 7 commits intoShamrock-code:mainfrom
tdavidcl:patch-2025-09-04-08-55

Conversation

@tdavidcl
Copy link
Member

@tdavidcl tdavidcl commented Sep 4, 2025

No description provided.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @tdavidcl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a new collective algorithm, are_all_rank_true, designed to determine if a boolean condition holds true across all MPI ranks. This addition enhances the library's capability for distributed logical operations, ensuring correctness through dedicated unit tests that cover various input scenarios.

Highlights

  • New Collective Operation: Introduced are_all_rank_true, a new collective function that returns true only if all participating MPI ranks provide a true boolean input.
  • MPI Implementation: The function leverages MPI_Allreduce with the MPI_LAND operation to efficiently determine the global logical AND of boolean inputs across ranks, including a special case for single-rank execution.
  • Unit Testing: Comprehensive unit tests have been added to validate the are_all_rank_true function under various conditions, including all true, all false, mixed inputs, and edge cases with specific ranks providing false.
  • Build System Integration: The CMake build system has been updated to include the new source file for the are_all_rank_true collective, ensuring it is properly compiled and linked.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds a new collective algorithm, are_all_rank_true, to perform a logical AND across all MPI ranks. The implementation is correct and includes a good set of unit tests. My feedback includes suggestions to improve documentation, remove unused code, and refactor the tests for better clarity and maintainability.

@tdavidcl
Copy link
Member Author

tdavidcl commented Sep 4, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new collective algorithm, are_all_rank_true, for performing a logical AND across all MPI ranks. The implementation is sound and includes a comprehensive set of unit tests. My review provides suggestions to enhance code clarity and maintainability in both the implementation and its tests. Specifically, I recommend using a more semantically appropriate MPI operation, removing an unused header, and refactoring the tests to eliminate code duplication and improve efficiency.

@tdavidcl
Copy link
Member Author

tdavidcl commented Sep 4, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new collective algorithm are_all_rank_true which performs a logical AND across all MPI ranks. The implementation is straightforward, but contains a potential correctness issue when used with MPI sub-communicators. I've provided a suggestion to fix this. Additionally, I've included some recommendations to improve the maintainability and conciseness of the new test code by refactoring duplicated logic and using standard library algorithms.

@tdavidcl
Copy link
Member Author

tdavidcl commented Sep 4, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds a new collective operation are_all_rank_true which performs a logical AND across all MPI ranks. The implementation is correct and uses MPI_Allreduce with MPI_LAND. The changes also include comprehensive unit tests for the new functionality.

My review includes a few suggestions for improvement:

  • In the implementation file, there's an unused header include that should be removed, and a boolean initialization that could be made more explicit.
  • In the test file, a helper lambda can be improved for performance by passing its vector argument by const reference, and for conciseness by using std::all_of.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2025

Workflow report

workflow report corresponding to commit 832b947
Commiter email is timothee.davidcleris@proton.me
GitHub page artifact URL GitHub page artifact link (can expire)

Pre-commit check report

Pre-commit check: ✅

trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check for merge conflicts................................................Passed
check that executables have shebangs.....................................Passed
check that scripts with shebangs are executable..........................Passed
check for added large files..............................................Passed
check for case conflicts.................................................Passed
check for broken symlinks................................................Passed
check yaml...............................................................Passed
detect private key.......................................................Passed
No-tabs checker..........................................................Passed
Tabs remover.............................................................Passed
Validate GitHub Workflows................................................Passed
clang-format.............................................................Passed
black....................................................................Passed
ruff check...............................................................Passed
Check doxygen headers....................................................Passed
Check license headers....................................................Passed
Check #pragma once.......................................................Passed
Check SYCL #include......................................................Passed
No ssh in git submodules remote..........................................Passed

Test pipeline can run.

Clang-tidy diff report

No relevant changes found.
Well done!

You should now go back to your normal life and enjoy a hopefully sunny day while waiting for the review.

Doxygen diff with main

Removed warnings : 0
New warnings : 0
Warnings count : 7689 → 7689 (0.0%)

Detailed changes :

@tdavidcl tdavidcl merged commit f6bcb77 into Shamrock-code:main Sep 4, 2025
48 checks passed
@tdavidcl tdavidcl deleted the patch-2025-09-04-08-55 branch September 4, 2025 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant