NCAR / ccpp-scm

CCPP Single Column Model
Other
13 stars 50 forks source link

Combination PR for #419, 428, 435, 445, 446, 458 #459

Closed grantfirl closed 2 months ago

grantfirl commented 2 months ago

Includes changes from:

419 - only necessary wildcard changes and removed the extract_FV3GFS_column_ic.py script (credit @bluefinweiwei)

428 - merged as-is (credit @hertneky)

435 - merged as-is (credit @dustinswales)

445 - renamed new suite SCM_GFS_v17_HR3, chose C192 namelist as the default for the suite (credit @bluefinweiwei)

446 - changes merged as-is (TODO: remove dependencies on all NCEPlibs?) (credit @hertneky)

458 - minor edits to affiliations (credit @scrasmussen )

grantfirl commented 2 months ago

@dustinswales Do we need some ccpp/physics changes to go along with the nvhpc build? It looks like the CI is erring out compiling the GF scheme. See https://github.com/NCAR/ccpp-physics/blob/6d8fccbea12c11387c0bc457dcb3855574cd29aa/physics/CONV/Grell_Freitas/cu_gf_deep.F90#L319 -- I think 'massflux' should be 'massflx'

grantfirl commented 2 months ago

@dustinswales Do we need some ccpp/physics changes to go along with the nvhpc build? It looks like the CI is erring out compiling the GF scheme. See https://github.com/NCAR/ccpp-physics/blob/6d8fccbea12c11387c0bc457dcb3855574cd29aa/physics/CONV/Grell_Freitas/cu_gf_deep.F90#L319 -- I think 'massflux' should be 'massflx'

@dustinswales It looks like your tests passed with the individual PR because it was using a slightly older commit of ccpp/physics. It looks like the error was introduced in https://github.com/NCAR/ccpp-physics/pull/1062. I think that I'd like to merge this as-is anyway and we can follow up with a fix later.

grantfirl commented 2 months ago

@dustinswales @mkavulich @bluefinweiwei @hertneky @scrasmussen I'm going to go ahead and merge this without an official approval since I reviewed all of the changes from you all and approve (this PR isn't really my work, so my approval should be sufficient).

dustinswales commented 2 months ago

@grantfirl Looking back to the physics PR where this openACC bug was introduced, we had a decent amount of discussion on how to review/handle these changes. We pulled in some GPU experts to review, but a small typo like this is hard to catch by the human eye (I know I didn't see it). Another reason why having a large arsenal of CI tests is so useful for reviewing.

grantfirl commented 2 months ago

@grantfirl Looking back to the physics PR where this openACC bug was introduced, we had a decent amount of discussion on how to review/handle these changes. We pulled in some GPU experts to review, but a small typo like this is hard to catch by the human eye (I know I didn't see it). Another reason why having a large arsenal of CI tests is so useful for reviewing.

Your work is already showing its usefulness! Instant gratification.

dustinswales commented 2 months ago

The NVIDIA test w/o openACC support should still pass, but it didn't. Weird? Rerunning now

XiaSun-Atmos commented 2 months ago

should the namelist input_GFS_v17_H3.nml be input_GFS_v17_HR3.nml?

grantfirl commented 2 months ago

should the namelist input_GFS_v17_H3.nml be input_GFS_v17_HR3.nml?

@XiaSun-Atmos This is fixed in #465