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 OMEGA Broadcast functions #36

Merged
merged 3 commits into from
Nov 18, 2023

Conversation

grnydawn
Copy link

This PR adds OMEGA broadcasting functions with documentation.

  • implements blocking broadcast functions
  • adds testings for the functions
  • adds user-guide and developer-guide

Checklist

  • Documentation:
    • Design document has been generated and added to the docs
    • User's Guide has been updated
    • Developer's Guide has been updated
    • Documentation has been built locally and changes look as expected
  • Testing
    • A comment in the PR documents testing used to verify the changes including any tests that are added/modified/impacted.
    • CTest unit tests for new features have been added per the approved design.
    • Polaris tests for new features have been added per the approved design (and included in a test suite)
    • [] Unit tests have passed. Please provide a relevant CDash build entry for verification.
    • Polaris test suite has passed
    • Performance related PRs: Please include a relevant PACE experiment link documenting performance before and after.
  • Stealth Features
    • If any stealth features are included in the PR, please confirm that they have been documented.

NOTE: Non-blocking broadcast functions are NOT included in this PR.

This PR adds OMEGA broadcasting functions with documentation.
* implements blocking broadcast functions
* adds testings for the functions
* adds user-guide and developer-guide
@grnydawn
Copy link
Author

@philipwjones , I have made modifications to two lines of the Broadcast design document, where the MachEnv function argument has been updated to a pointer variable.

Copy link

@philipwjones philipwjones left a comment

Choose a reason for hiding this comment

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

Other than the changes noted, this looks good and passes tests on Chrysalis. I'll try building the docs and re-test when the rank and style changes are done.


//------------------------------------------------------------------------------
// Broadcast I4 scalar value
int Broadcast(I4 &value, const MachEnv *InEnv, const int rankBcast) {

Choose a reason for hiding this comment

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

Following the style guide, all vars start with a capital letter (I still have trouble with this too as evidenced by the design doc). So Value, RankBcast and Retval/RetVal throughout.

Copy link
Author

Choose a reason for hiding this comment

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

renamed variables as suggested.

namespace OMEGA {

// blocking broadcast scalar
int Broadcast(I4 &value, const MachEnv *InEnv = MachEnv::getDefaultEnv(),

Choose a reason for hiding this comment

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

One big change here. We allow the MachEnv to define a MasterTask other than 0, so in all these interfaces, we want to set the argument RankBcast to an invalid value and extract MasterTask from the MachEnv.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks a lot for pointing this. Now, if user does not specify RankBcast value(default value is set to an invalid number of "-1"), the module get it using getMasterTask().

@philipwjones
Copy link

A few more changes after building the docs. Remember to add the Broadcast docs to the top-level index.md file.

Also, could you make all the headings other than the main broadcast heading a level lower than the main heading. Right now, many of the subsections are showing up as top-level sections in the contents.

@philipwjones
Copy link

One last (?) thing - I just realized these are in the infra directory. Can you please move them over to the base directory? That's where much of the MPI-related infrastructure should sit. The infra directory should be for utilities that are less dependent on the MPI model (eg Config, Logging, TimeManager, etc). That means also moving the docs, etc to match the change in location. Thanks.

* properly use MasterRank in MachEnv instead of rank=0
* renamed variable names to match with Clang style
* updated document accordingly
* moved Broadcast mode from infra to base directory
* fixed bugs in document
@grnydawn
Copy link
Author

@philipwjones Thanks a lot for the suggestions. I think I have applied all of your suggestions, and please review if the changes match with your thought.

Copy link

@philipwjones philipwjones left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes. I have confirmed unit tests pass on both Chrysalis (intel) and Frontier (crayclangcpu). I also built the documentation and it looks fine.

Copy link

@xylar xylar left a comment

Choose a reason for hiding this comment

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

I'm approving just based on a visual inspection of the docs. I don't have time at the moment to do a code review, sorry about that. But I think @philipwjones' review is sufficient.

@philipwjones philipwjones merged commit 627cff4 into E3SM-Project:develop Nov 18, 2023
2 checks passed
amametjanov pushed a commit that referenced this pull request Dec 5, 2023
@grnydawn grnydawn deleted the ykim/omega/broadcast-pr branch April 30, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants