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

Fixes to allow FV3_HRRR_c3 to run with gnu debug plus PR #113, #106, and #103 #102

Merged
merged 17 commits into from
Oct 3, 2023

Conversation

SamuelTrahanNOAA
Copy link
Collaborator

@SamuelTrahanNOAA SamuelTrahanNOAA commented Sep 8, 2023

The c3 code cannot handle null pointers correctly. This changes some array argument declarations so tests pass. At the ufs-weather-model level, I've added hrrr_c3_debug tests for both the gnu and intel compilers. The new tests pass on Jet, Hera, and Cheyenne.

This PR includes #113 and #106

@lisa-bengtsson
Copy link
Collaborator

Changes look good, but it is a bit surprising the the cu_gf_deep.F90 doesn't run into the same problem?

@SamuelTrahanNOAA
Copy link
Collaborator Author

Changes look good, but it is a bit surprising the the cu_gf_deep.F90 doesn't run into the same problem?

Correct. I didn't change other schemes because I wanted to simplify the changes as much as possible.

My ufs-weather-model changes add hrrr_c3_debug and hrrr_gf_debug tests for both compilers. If we encounter problems again, we'll know.

@SamuelTrahanNOAA
Copy link
Collaborator Author

In a recent email about this PR, I got the usual question about WRF compatibility, so I'm pasting my answer here.

WRF needs a starting index for each dimension. By that, I mean the its: part of its:ite. If you remove both the its and the ite (dimension(:)) then the starting index is 1. That breaks WRF. However, if you keep the starting index (dimension(its:)) then the array starts at its and WRF will work. If you include the ite then CCPP will crash with the gnu compiler if you pass a null pointer.

In short, your dimensions must look like this

dimension(its:,kts:) :: my_3d_array
dimension(its:) :: my_2d_array
dimension(kts:) :: my_column_array

That way, both WRF and CCPP work.

@dustinswales
Copy link
Collaborator

In a recent email about this PR, I got the usual question about WRF compatibility, so I'm pasting my answer here.

WRF needs a starting index for each dimension. By that, I mean the its: part of its:ite. If you remove both the its and the ite (dimension(:)) then the starting index is 1. That breaks WRF. However, if you keep the starting index (dimension(its:)) then the array starts at its and WRF will work. If you include the ite then CCPP will crash with the gnu compiler if you pass a null pointer.

In short, your dimensions must look like this

dimension(its:,kts:) :: my_3d_array
dimension(its:) :: my_2d_array
dimension(kts:) :: my_column_array

That way, both WRF and CCPP work.

@SamuelTrahanNOAA Your are spot on.

@dudhia
Copy link

dudhia commented Sep 14, 2023

@dustinswales does it actually break WRF? We just like to keep the absolute index for debugging purposes.

@dustinswales
Copy link
Collaborator

e absolute index for debugging purposes.

@dudhia
@SamuelTrahanNOAA is the expert on null pointers :)
But from my understanding,
changing from its:ite -> : will break the WRF code, but the CCPP will work
changing from its:ite -> its: will work for both hosts.

@SamuelTrahanNOAA
Copy link
Collaborator Author

does it actually break WRF? We just like to keep the absolute index for debugging purposes.

Loops inside the WRF physics use its as a starting index. If the arrays start at 1 and you access index its, it may be far beyond the allocated array. The its:ite may be 2560:2610. Lose the its and the array bounds will be 1:51. Accessing index its=2560 puts you past the array bounds.

Also, WRF arrays are declared with memory bounds, not tile bounds:

dimension(ims:ime,kms:kme,jms:jme)

So long WRF passes arrays to the low-level physics routines like this, it'll work:

call thing(grid%var(its:ite,kts:kte,jts:jte))
or
call thing(var(its:ite,kts:kte,jts:jte))

@dudhia
Copy link

dudhia commented Sep 14, 2023

@SamuelTrahanNOAA I see. Yes, we can't do that in WRF as we also pass in its. CCPP must be treating those as 1.

@SamuelTrahanNOAA
Copy link
Collaborator Author

I've merged #106 into this PR since the UFS code managers plan to merge the two simultaneously.

@BrianCurtis-NOAA
Copy link

Can we get at least one re-approval since there have been code changes since the latest approval. Just need someone to be OK with those newer changes.

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.

@BrianCurtis-NOAA This still looks good to me after the combination of PRs.

@SamuelTrahanNOAA SamuelTrahanNOAA changed the title Fixes to allow FV3_HRRR_c3 to run with gnu debug Fixes to allow FV3_HRRR_c3 to run with gnu debug plus PR #113 and #106 Sep 29, 2023
@SamuelTrahanNOAA SamuelTrahanNOAA changed the title Fixes to allow FV3_HRRR_c3 to run with gnu debug plus PR #113 and #106 Fixes to allow FV3_HRRR_c3 to run with gnu debug plus PR #113, #106, and #103 Sep 29, 2023
@SamuelTrahanNOAA
Copy link
Collaborator Author

@haiqinli - Please confirm I merged your changes correctly.

@haiqinli
Copy link
Collaborator

@SamuelTrahanNOAA Thank you very much! Your merging looks good to me.

@jkbk2004
Copy link

jkbk2004 commented Oct 3, 2023

@grantfirl @dustinswales All tests are done at ufs-community/ufs-weather-model#1893. Can you merge this pr?

@grantfirl grantfirl merged commit dd91c3a into ufs-community:ufs/dev Oct 3, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants