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 to ensure RRTMGP for use on multiple threads. #568

Merged
merged 14 commits into from
Feb 24, 2021

Conversation

dustinswales
Copy link
Collaborator

@dustinswales dustinswales commented Feb 16, 2021

This PR contains several changes to the initialization and setup of the RRTMGP radiation scheme to allow for use across multiple openMP threads. Additionally, MPI commands were added to the initialization routines.
*) Moved Interstitial RRTMGP DDTs (ty_rrtmgp_gas_optics, ty_cloud_optics) to static fields defined during initialization in module memory.
*) Add "$omp critical" statements around the calling type-bound procedures during initialization.
*) Move allocation of ty_gas_conc from Interstitial to GFS_typedefs. Initialize ty_gas_concs for all blocks during initialization.

The GP regression tests on hera using this branch were successful.

Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

Changes look good.

I don't understand why the $omp critical statements are needed, because by definition/CCPP requirement the _init phases are called outside of threaded environments. You can use threading inside the _init routine, provided that you do things like $OMP parallel num_threads(my_num_threads) where my_num_threads is a variable similar to mpicomm, but the call to the _init routine is definitely not happening by multiple threads.

At any rate, those $omp critical statements don't hurt, they may at worst cause confusion.

Will approve once the regression tests passed on all platforms.

physics/GFS_rrtmgp_sw_post.F90 Outdated Show resolved Hide resolved
physics/GFS_rrtmgp_sw_pre.F90 Outdated Show resolved Hide resolved
physics/rrtmgp_lw_cloud_optics.F90 Outdated Show resolved Hide resolved
physics/rrtmgp_lw_gas_optics.F90 Outdated Show resolved Hide resolved
physics/rrtmgp_lw_rte.F90 Outdated Show resolved Hide resolved
physics/rrtmgp_sw_cloud_optics.F90 Outdated Show resolved Hide resolved
physics/rrtmgp_sw_cloud_sampling.F90 Outdated Show resolved Hide resolved
physics/rrtmgp_sw_gas_optics.F90 Outdated Show resolved Hide resolved
@dustinswales
Copy link
Collaborator Author

Changes look good.

I don't understand why the $omp critical statements are needed, because by definition/CCPP requirement the _init phases are called outside of threaded environments. You can use threading inside the _init routine, provided that you do things like $OMP parallel num_threads(my_num_threads) where my_num_threads is a variable similar to mpicomm, but the call to the _init routine is definitely not happening by multiple threads.

At any rate, those $omp critical statements don't hurt, they may at worst cause confusion.

Will approve once the regression tests passed on all platforms.

@climbfuji
I've made the requested changes. I'm currently running a test to see if I can remove the omp directive from the initialization. (They could be a relic of my days noodling around trying to get the code to work on multiple threads...)

@dustinswales
Copy link
Collaborator Author

@climbfuji
Just ran a test with the omp directives removed and it worked fine on multiple threads.

@@ -7,29 +7,84 @@ module rrtmgp_lw_gas_optics
use mo_optical_props, only: ty_optical_props_1scl
use mo_compute_bc, only: compute_bc
use rrtmgp_aux, only: check_error_msg
use GFS_rrtmgp_pre, only: active_gases_array
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems a bit weird to me to be passing in something to RRTMGP internal code from a GFS-based interstitial scheme. Seems like this could affect future portability, right? Is this necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, but I couldn't get the code to work w/ multiple threads with active_gases_array defined as an Interstitial.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the reply. If it's critical to what the PR is trying to fix, then no problem. Maybe there will be a different solution in the future. I'm just thinking in terms of building a non-GFS-based suite with RRTMGP in the future. It would require some modifications to deal with this. That bridge can be crossed when the time comes.

Copy link
Collaborator

@grantfirl grantfirl left a comment

Choose a reason for hiding this comment

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

I didn't see anything that raises my hackles CCPP-wise except for one comment that I made about passing in something via module from a GFS interstitial scheme.

Copy link
Contributor

@llpcarson llpcarson left a comment

Choose a reason for hiding this comment

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

Approved

@climbfuji climbfuji merged commit 869dfaa into NCAR:master Feb 24, 2021
@dustinswales dustinswales deleted the hotfix_GPthreadsafe branch March 4, 2021 22:58
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.

4 participants