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

Branch dev/emc: convert GFS DDTs from blocked data structures to contiguous arrays #330

Conversation

climbfuji
Copy link

@climbfuji climbfuji commented Mar 11, 2024

Description

In the UFS atmospheric component fv3atm and its submodules, convert internal GFS DDTs from blocked data structures to contiguous arrays. This excludes the (external) GFS_extdiag and GFS_restart DDTs. Also remove the GFS_Data/IPD_Data super DDT and use the individual DDTs directly.

Per discussion in #314, the file that is modified here (driver/fvGFS/atmosphere.F90) is separate for each branch in the FV3 dycore repo.

I've tried to make minimal changes only, a lot more cleanup can be done in a future PR:

  1. Since each branch has its own atmosphere.F90, we can do away with a lot of the #ifdef statements
  2. Many of the loops that calculate nb, ix and the new im can be simplied to just calculate im or use reshape.

The set of PRs listed below address ufs-community/ufs-weather-model#2294

** Associated PRs**

#330
ufs-community/ccpp-physics#183
NOAA-EMC/fv3atm#798
ufs-community/ufs-weather-model#2183

How Has This Been Tested?

With the Unified Forecast System. Results are bit for bit identical with the current baseline.

Checklist:

Please check all whether they apply or not

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@climbfuji climbfuji changed the title (sneak preview only) Branch dev/emc: convert GFS DDTs from blocked data structures to contiguous arrays Branch dev/emc: convert GFS DDTs from blocked data structures to contiguous arrays May 22, 2024
@climbfuji climbfuji marked this pull request as ready for review May 22, 2024 20:18
@climbfuji
Copy link
Author

@bensonr This PR is finally ready for review - sorry for the long wait.

Copy link
Contributor

@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.

If you are no longer using the blocking in the same way as previous, why not remove all of the do nb=1,blen loops and replace them with do im=1,size(compute_domain)?

im = IPD_control%chunk_begin(nb)+ix-1
u_dt(i,j,k1) = u_dt(i,j,k1) + (IPD_Stateout%gu0(im,k) - IPD_Statein%ugrs(im,k)) * rdt
v_dt(i,j,k1) = v_dt(i,j,k1) + (IPD_Stateout%gv0(im,k) - IPD_Statein%vgrs(im,k)) * rdt
! t_dt(i,j,k1) = (IPD_Stateout%gt0(im,k) - IPD_Statein%tgrs(im,k)) * rdt
Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to remove this commented line as it doesn't add to understanding of the tendency in any way.

driver/fvGFS/atmosphere.F90 Show resolved Hide resolved
@climbfuji
Copy link
Author

If you are no longer using the blocking in the same way as previous, why not remove all of the do nb=1,blen loops and replace them with do im=1,size(compute_domain)?

That is indeed the plan in a follow-up PR. My goal was to get b4b identical results to increase confidence in the changes - and it worked. All 400-ish regression tests are b4b identical on Hera and Hercules (with Intel and GNU).

@jkbk2004
Copy link

We are going to start testing ufs-community/ufs-weather-model#2183. Please, go ahead for final reviews and approvals.

@jkbk2004
Copy link

@climbfuji it will be good option to combine in #344 sine no need to change baseline. can we do that?

@climbfuji
Copy link
Author

@climbfuji it will be good option to combine in #344 sine no need to change baseline. can we do that?

Sorry, but I really think the changes I am making across repositories are large and substantial enough that they should remain in their own set of PRs. The CI addition in fv3atm is an exception, since it doesn't touch any code but just github action stuff.

@bensonr
Copy link
Contributor

bensonr commented Jul 3, 2024

@climbfuji - are you still planning to rename the fvgfs directory as part of this update (see issue #314)?

@climbfuji
Copy link
Author

@climbfuji - are you still planning to rename the fvgfs directory as part of this update (see issue #314)?

I definitely forgot about that, sorry. There's a bit of a hold up at the moment, since we are debugging problems with the UFS update that includes this PR, and since my time is really limited. I don't know how quickly we get through this, but when we do and if you still think it's a good idea to rename fvGFS to UFS, then I'll make the change.

@bensonr
Copy link
Contributor

bensonr commented Jul 3, 2024

@climbfuji - are you still planning to rename the fvgfs directory as part of this update (see issue #314)?

I definitely forgot about that, sorry. There's a bit of a hold up at the moment, since we are debugging problems with the UFS update that includes this PR, and since my time is really limited. I don't know how quickly we get through this, but when we do and if you still think it's a good idea to rename fvGFS to UFS, then I'll make the change.

To publicize this far and wide, I'll mention this being a planned part of your PR at the next UFS code managers meeting

@laurenchilutti
Copy link
Contributor

This branch has conflicts that must be resolved before we can merge. Can you merge the dev/emc branch into your feature branch and resolve any conflicts?

@climbfuji
Copy link
Author

This branch has conflicts that must be resolved before we can merge. Can you merge the dev/emc branch into your feature branch and resolve any conflicts?

@laurenchilutti We are currently debugging issues in the UFS with the set of PRs that this PR belongs to. I also still need to rename fvGFS to UFS as discussed above. I'll update the branch from dev/emc when these issues are resolved. Please wait with your review until then - thanks for your patience!

…yfile, driver/UFS/atmosphere.F90, model/fv_arrays.F90, tools/fv_nudge.F90; also resolve a previous merge conflict that was left in docs/Doxyfile
@climbfuji
Copy link
Author

@climbfuji - are you still planning to rename the fvgfs directory as part of this update (see issue #314)?

I definitely forgot about that, sorry. There's a bit of a hold up at the moment, since we are debugging problems with the UFS update that includes this PR, and since my time is really limited. I don't know how quickly we get through this, but when we do and if you still think it's a good idea to rename fvGFS to UFS, then I'll make the change.

To publicize this far and wide, I'll mention this being a planned part of your PR at the next UFS code managers meeting

@bensor I made the change from fvGFS to UFS. I renamed the directory and also changed the occurences of fvGFS in the files where it seemed to be reasonable. I found a merge conflict that was left from a previous PR in docs/Doxyfile and resolved it to the best of my knowledge. I am running full regression tests with the UFS for the updated code - results will be reported in ufs-community/ufs-weather-model#2183.

@FernandoAndrade-NOAA
Copy link

Testing on 2183 has completed successfully, please continue with the merge process.

@laurenchilutti
Copy link
Contributor

@XiaqiongZhou-NOAA Can you review this before I merge?

@laurenchilutti laurenchilutti merged commit 7c3102f into NOAA-GFDL:dev/emc Aug 8, 2024
@laurenchilutti
Copy link
Contributor

Testing on 2183 has completed successfully, please continue with the merge process.

Done!

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.

6 participants