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

Implementation of CCPP timestep_init and timestep_final phases #47

Merged
merged 4 commits into from
Jan 8, 2021

Conversation

climbfuji
Copy link

@climbfuji climbfuji commented Dec 15, 2020

This PR contains the following changes:

  • Implementation of CCPP timestep_init and timestep_final phases in model/fv_dynamics.F90

Note that while these new phases are currently not doing any work, they are required for transitioning to the new CCPP code generator capgen.py (scheduled for February 2021), at which time they will be taking over some of the work that is currently done manually (allocating data for the CCPP fast physics calls).

Associated PRs:

ufs-community/ufs-weather-model#337
NOAA-EMC/fv3atm#217
NCAR/ccpp-physics#534
NCAR/ccpp-framework#344
#47

For regression testing information, see ufs-community/ufs-weather-model#337.

@climbfuji
Copy link
Author

I am opening this PR for review. I will have to pull in updates from NOAA-EMC dev/emc in case anything gets committed until it is the turn for this PR (I am only aware of the "remove mpp_node" PR). But the code changes itself should be final.

@climbfuji
Copy link
Author

@junwang-noaa @DusanJovic-NOAA please add reviewers, thanks!

Copy link
Collaborator

@XiaqiongZhou-NOAA XiaqiongZhou-NOAA left a comment

Choose a reason for hiding this comment

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

I do not understand CCPP completely, but still feel the changes are OK.

Copy link
Collaborator

@bensonr bensonr left a comment

Choose a reason for hiding this comment

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

My understanding is the timestep init and finalize replace the physics time-varying pre-step global operations. It also appears these same functions now have stages called out by the particular group_name. I assume these functions are being added here for consistency and not because the fast_physics group is actually performing any pre-physics global operations - is that correct?

To futureproof for when others add more physical parameterizations at the "fast_physics" timescale but with no dynamical core dependencies, is this the proper place to be calling the timestep init and finalize?

@climbfuji
Copy link
Author

My understanding is the timestep init and finalize replace the physics time-varying pre-step global operations. It also appears these same functions now have stages called out by the particular group_name. I assume these functions are being added here for consistency and not because the fast_physics group is actually performing any pre-physics global operations - is that correct?

Yes, that is correct.

To futureproof for when others add more physical parameterizations at the "fast_physics" timescale but with no dynamical core dependencies, is this the proper place to be calling the timestep init and finalize?

We (DTC CCPP developers) don't really have any knowledge of how/where exactly additional fast physics processes will be added. It seems likely, though, that they will be inside the actual call to the dynamics (fv_dynamics), in which case calling timestep_init and timestep_final at the beginning and end of the routine makes sense to me.

But please correct me if my understanding is incorrect and you think there are more suitable places for these calls.

@bensonr
Copy link
Collaborator

bensonr commented Jan 7, 2021

@climbfuji - I don't think there is a more suitable place at this point in time. I'll give it another look-see and post a new review.

@DusanJovic-NOAA DusanJovic-NOAA merged commit 34a6fdf into NOAA-EMC:dev/emc Jan 8, 2021
aerorahul pushed a commit that referenced this pull request Jul 17, 2021
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