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

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

Merged
merged 133 commits into from
Apr 12, 2024

Conversation

grantfirl
Copy link
Collaborator

@grantfirl grantfirl commented Apr 11, 2024

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 grantfirl changed the title Combination PR for #419, Combination PR for #419, 435, Apr 11, 2024
@grantfirl grantfirl changed the title Combination PR for #419, 435, Combination PR for #419, 435, 445 Apr 11, 2024
@grantfirl grantfirl changed the title Combination PR for #419, 435, 445 Combination PR for #419, 435, 445, 446 Apr 11, 2024
@grantfirl grantfirl changed the title Combination PR for #419, 435, 445, 446 Combination PR for #419, 435, 445, 446, 458 Apr 11, 2024
@grantfirl grantfirl mentioned this pull request Apr 11, 2024
@grantfirl grantfirl changed the title Combination PR for #419, 435, 445, 446, 458 Combination PR for #419, 428, 435, 445, 446, 458 Apr 11, 2024
@grantfirl grantfirl marked this pull request as ready for review April 11, 2024 20:34
@grantfirl
Copy link
Collaborator Author

grantfirl commented Apr 11, 2024

@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
Copy link
Collaborator Author

@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 NCAR/ccpp-physics#1062. I think that I'd like to merge this as-is anyway and we can follow up with a fix later.

@grantfirl
Copy link
Collaborator Author

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

@grantfirl grantfirl merged commit bd3128c into NCAR:main Apr 12, 2024
17 of 21 checks passed
@dustinswales
Copy link
Collaborator

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator

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

@XiaSun-Atmos
Copy link

should the namelist input_GFS_v17_H3.nml be input_GFS_v17_HR3.nml?

@grantfirl
Copy link
Collaborator Author

should the namelist input_GFS_v17_H3.nml be input_GFS_v17_HR3.nml?

@XiaSun-Atmos This is fixed in #465

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.

3 participants