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

update doc files #118

Merged
merged 1 commit into from
Sep 3, 2024
Merged

update doc files #118

merged 1 commit into from
Sep 3, 2024

Conversation

aekiss
Copy link
Contributor

@aekiss aekiss commented Sep 2, 2024

contributes to COSIMA/access-om3#224

Copy link

@minghangli-uni minghangli-uni left a comment

Choose a reason for hiding this comment

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

Thanks @aekiss LGTM! Only a comment on the selection of block_size_x, block_size_y, and max_blocks.

distribution_type = "cartesian"
distribution_wght = "latitude"
maskhalo_bound = .true.
maskhalo_dyn = .true.
maskhalo_remap = .true.
max_blocks = 10
max_blocks = 15

Choose a reason for hiding this comment

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

Suggested change
max_blocks = 15
max_blocks = -1

In the latest 025deg_jra55do_ryf, max_blocks has been set to -1 to allow the code to automatically determine the optimal value.

max_blocks = -1

I can quickly update the 1deg config.

Comment on lines +71 to +72
block_size_x = 15
block_size_y = 20

Choose a reason for hiding this comment

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

As was discussed in COSIMA/access-om3#214 (comment) and cice docs, It is suggested that max_blocks be set in the range of 3-8 blocks per processor.

Suggested change
block_size_x = 15
block_size_y = 20
block_size_x = 30
block_size_y = 40

With the updated block_size_x and block_size_y, max_blocks is determined to be 8, which suits the recommendation from the cice docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

With the slender distributions block_size_x should equal nx_global/nprocs i.e. 360/24 = 15. Which is why these block sizes are set as they are at the moment

Choose a reason for hiding this comment

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

Thanks @anton-seaice. So for slenderX1, the block size in the y direction should be 300 instead of 20, right? This is because there is only one processor in the y direction, and it needs to handle all 300 grid points.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could try that. That would mean there is 1 block per processor, which means you couldn't eliminate any blocks which are entirely land. I am not sure how it could impact inter-process communication ? I suspect the performance will be about the same as the current configuration ?

Choose a reason for hiding this comment

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

I dont get it. If that’s the case, why does block_size_x equal nx_global / nprocs, but block_size_y is calculated differently?

Copy link
Contributor

Choose a reason for hiding this comment

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

Its just kind of by definition, you can have multiple blocks in Y but not X. Each color is a different processor:

Screenshot 2024-09-03 at 1 36 33 PM

@aekiss
Copy link
Contributor Author

aekiss commented Sep 3, 2024

This PR is just to update the documentation to match the config, so how about we merge this, then explore changes to the config as a separate issue? It's easy to rerun update_doc.sh anytime to sync the docs again.

Copy link

@minghangli-uni minghangli-uni left a comment

Choose a reason for hiding this comment

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

Sure @aekiss. Feel free to merge it.

@aekiss aekiss merged commit 7de31c4 into main Sep 3, 2024
@aekiss aekiss deleted the update-doc.iss224 branch September 3, 2024 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants