-
Notifications
You must be signed in to change notification settings - Fork 7
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
Deprecate nprocs, and better estimate for max_blocks #149
Conversation
…e_in Co-authored-by: minghangli-uni <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Just a few suggestions, mostly optional.
CICE/patches/ice_domain.F90.patch
Outdated
|
||
endif | ||
|
||
+ if (nprocs .ne. -1) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ if (nprocs .ne. -1) then | |
+ if (nprocs /= -1) then |
Easier to read and seems to be the style favored in the rest of the module.
CICE/patches/ice_domain.F90.patch
Outdated
+ if (nprocs .ne. -1) then | ||
+ if (my_task == master_task) then | ||
+ write(nu_diag,*) subname//' WARNING: nprocs is deprecated, please remove from namelist' | ||
+ endif | ||
+ endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could move this to be inside the above if
statement, so that there's no need to check again if my_task == master_task
.
CICE/patches/ice_domain.F90.patch
Outdated
+ | ||
+ nprocs = get_num_procs() | ||
+ | ||
call broadcast_scalar(nprocs, master_task) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to broadcast the number of procs anymore, as all procs are now getting the value with the call to get_num_procs
.
CICE/patches/ice_domain.F90.patch
Outdated
call broadcast_scalar(add_mpi_barriers, master_task) | ||
call broadcast_scalar(debug_blocks, master_task) | ||
+ | ||
+ ! update nprocs_x and nprocs_y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would write this in a slightly different way:
! Broadcast data from namelist
call broadcast_scalar(processor_shape, master_task)
call broadcast_scalar(distribution_type, master_task)
call broadcast_scalar(distribution_wght, master_task)
call broadcast_scalar(distribution_wght_file, master_task)
call broadcast_scalar(ew_boundary_type, master_task)
call broadcast_scalar(ns_boundary_type, master_task)
call broadcast_scalar(maskhalo_dyn, master_task)
call broadcast_scalar(maskhalo_remap, master_task)
call broadcast_scalar(maskhalo_bound, master_task)
call broadcast_scalar(add_mpi_barriers, master_task)
call broadcast_scalar(debug_blocks, master_task)
call broadcast_scalar(max_blocks, master_task)
call broadcast_scalar(block_size_x, master_task)
call broadcast_scalar(block_size_y, master_task)
call broadcast_scalar(nx_global, master_task)
call broadcast_scalar(ny_global, master_task)
nprocs = get_num_procs()
! update nprocs_x and nprocs_y
call proc_decomposition(nprocs, nprocs_x, nprocs_y)
! Automatically determine max_blocks if requested by user
if (max_blocks < 1) then
max_blocks=((nx_global-1)/block_size_x/nprocs_x+1) * &
((ny_global-1)/block_size_y/nprocs_y+1)
max_blocks=max(1,max_blocks)
if (my_task == master_task) then
write(nu_diag,'(/,a52,i6,/)') &
'(ice_domain): max_block < 1: max_block estimated to ',max_blocks
endif
endif
I think this is easier to read, but in the end this a personal preference, so feel free to ignore this suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Micael
So broadcast_scalar
calls will wait for that variable to be populated before continuing ?
i.e. On PEs which are not master_task, will this will wait for block_size and max_blocks to be read from the namelist before the max_blocks calculation starts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e. On PEs which are not master_task, will this will wait for block_size and max_blocks to be read from the namelist before the max_blocks calculation starts?
Yes, exactly. Each task will only return from the call to broadcast_scalar
and continue execution once it has a local copy of the value being broadcasted.
Looks good to me. Thank you @anton-seaice. |
Made the changes proposed and tested with 1def_iaf and max_blocks not set:
|
Deprecate nprocs, and better estimate for max_blocks if not set in ice_in
Closes #145