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

Changes for CICECORE load balance #626

Merged
merged 4 commits into from
Sep 8, 2022
Merged

Conversation

aoloso
Copy link
Contributor

@aoloso aoloso commented Aug 31, 2022

Changes were made to CICECORE in GEOS_CICE4ColumnPhysGridComp.F90 for load balancing.

@aoloso aoloso added the 0 diff structural Structural changes to repository that are zero-diff label Aug 31, 2022
@aoloso aoloso requested review from a team as code owners August 31, 2022 20:36
zhaobin74
zhaobin74 previously approved these changes Aug 31, 2022
Copy link
Contributor

@zhaobin74 zhaobin74 left a comment

Choose a reason for hiding this comment

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

This has been tested with zero-diff results.

Copy link
Contributor

@sanAkel sanAkel left a comment

Choose a reason for hiding this comment

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

@aoloso would you please add some documentation (preferably inline, in GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/GEOSsaltwater_GridComp/BufferPacking.h and in GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/GEOSsaltwater_GridComp/BufferUnpacking.h) to describe what is:

  • purpose
  • how it is being accomplished

If you do not like to add that in the code, please have it in the PR "comments" section.

Thank you!

@mathomp4
Copy link
Member

mathomp4 commented Sep 1, 2022

I believe this requires GEOS-ESM/MAPL#1650, right? If so, we should block this until a new MAPL is released.

@zhaobin74 zhaobin74 added the Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) label Sep 1, 2022
@zhaobin74
Copy link
Contributor

I believe this requires GEOS-ESM/MAPL#1650, right? If so, we should block this until a new MAPL is released.

@mathomp4, yes, it depends on that one. DNA label added.

@aoloso
Copy link
Contributor Author

aoloso commented Sep 1, 2022

@sanAkel
I have added comments to GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/GEOSsaltwater_GridComp/[BufferPacking.h, BufferUnpacking.h] as you suggested.

@zhaobin74
Copy link
Contributor

zhaobin74 commented Sep 1, 2022

@aoloso, since LB_ON is zero-diff with LB_OFF but runs faster, how about making ON as default in the code? Could you do that?

Copy link
Contributor

@sanAkel sanAkel left a comment

Choose a reason for hiding this comment

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

Thank you @aoloso

@sanAkel sanAkel removed the Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) label Sep 1, 2022
@sanAkel
Copy link
Contributor

sanAkel commented Sep 1, 2022

@aoloso, since LB_ON is zero-diff with LB_OFF but runs faster, how about making ON as default in the code? Could you do that?

I support this request!

@aoloso
Copy link
Contributor Author

aoloso commented Sep 1, 2022

@sanAkel I made it "TRUE" by default in this pull request. It was "FALSE" by default only for development when working off of "aogcm" branch.

@zhaobin74
Copy link
Contributor

@sanAkel I made it "TRUE" by default in this pull request. It was "FALSE" by default only for development when working off of "aogcm" branch.

Thanks you, @aoloso.

@mathomp4 mathomp4 requested a review from sdrabenh September 1, 2022 19:50
@mathomp4
Copy link
Member

mathomp4 commented Sep 1, 2022

Okay. MAPL 2.25 has been released:

https://github.com/GEOS-ESM/MAPL/releases/tag/v2.25.0

I suppose what is next is to have someone try this code with that tag. I'll make a PR to GEOSgcm to get MAPL 2.25 in there.

@sdrabenh sdrabenh merged commit bc1c1b5 into develop Sep 8, 2022
@sdrabenh sdrabenh deleted the feature/aoloso/load_balance branch September 8, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff structural Structural changes to repository that are zero-diff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants