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

Stochastic physics cleanup #47

Merged
merged 39 commits into from
Sep 30, 2021
Merged

Conversation

pjpegion
Copy link
Collaborator

This pull request includes a massive cleanup of the stochastic physics code, addition of a new random number generator for the CA code which allows for the same answer for different PE layouts.
Adds restart capability for ca_global, and unit test programs in subdirectory unit_test, which currently only works on hera.
Also, iseed_ca is now an 8-bit integer, so the seed in the workflow can used the same methodology as iseed_sppt, etc.

This also fixes
#33
and
#25

@lisa-bengtsson
Copy link
Collaborator

The code addresses the CA MPI reproducibility issue:
ufs-community/ufs-weather-model#743
Provides a better solution for the CA restart as discussed:
ufs-community/ufs-weather-model#797
Fixes the CA GNU debug issue:
ufs-community/ufs-weather-model#770

Copy link
Collaborator

@lisa-bengtsson lisa-bengtsson left a comment

Choose a reason for hiding this comment

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

Hi Phil! Thanks for this, a lot of work has gone in to this one clearly! Regarding the cleanup of the stochastic physics code, I don't know much about the capabilities of the removed code, so I don't have many comments on that piece. But I assume that you know if anyone outside of PSL would be dependent on any of those features?

Regarding the CA code, in update_ca.F90 with this line:
!Return if not allocated:
if(.not. allocated(board) .and. .not. allocated(lives) .and. .not. allocated(board_g) .and. .not. allocated(lives_g))return

What happens if you only run ca_sgs? Or only ca_global?

A small remark, in the global_ca part of update_ca.F90 , newcell=0 looks to be set twice.

It looks like there is a fv3atm submodule update in the stochastic_physics_wrapper.F90 (and atmos_model.F90?) that goes with this PR? Could be good to post that number in the description.

@pjpegion
Copy link
Collaborator Author

  1. most of the deleted files have been moved into spectral_transforms.F90, and are still available.
  2. re: code in update_ca.F90, I will take a look. It looked like things were fine if I ran only ca_global or ca_sgs, but will check again
  3. I will remove the duplicate newcell = 0
  4. I am going to merge in the ocean stochastic physics PR, which also touches fv3atm, so I haven't issued that PR yet. When I do, I will post it here.

@pjpegion
Copy link
Collaborator Author

the line
if(.not. allocated(board) .and. .not. allocated(lives) .and. .not. allocated(board_g) .and. .not. allocated(lives_g))return
will only return if nothing is allocated.

@pjpegion
Copy link
Collaborator Author

One thing to note is that all of the code clean-up with the spectral transforms does not change the baseline results for the stochastic physics or land perturbation tests.

@lisa-bengtsson
Copy link
Collaborator

I'm a little curious about the io_layout and need for the io domain specification, does it have to be separate from the ca compute domain?

@pjpegion
Copy link
Collaborator Author

That is needed for the new way of reading and writing restarts.

@pjpegion
Copy link
Collaborator Author

associated fv3atm PR is NOAA-EMC/fv3atm#396 and
ccpp-physics PR NCAR/ccpp-physics#737

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.

Not a thorough review, I only skimmed through the code changes

@pjpegion pjpegion merged commit 57fa71e into NOAA-PSL:master Sep 30, 2021
@pjpegion pjpegion deleted the unit_tests_orion branch September 30, 2021 17:50
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