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

~~Add NRL_parse_tracers.F90 as a replacement for GFDL_parse_tracers.F90 for NEPTUNE~~ Remove GFDL_parse_tracers.F90 #1087

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

climbfuji
Copy link
Collaborator

@climbfuji climbfuji commented Sep 19, 2024

This PR has been updated based on the discussion below. It now removes GFDL_parse_tracers.F90 and all mentions of it.

Original PR description

This PR cherry-picks an update from the NRL fork that adds NRL_parse_tracers.F90 as an alternative for GFDL_parse_tracers.F90 for NEPTUNE. It translates the tracer names used in GFS_typedefs (stemming from the FV3 dycore at GFDL) into the correct names used at NRL, otherwise it is the same as the GFDL version.

The motivation for doing it this way is so that the code in NRL's GFS_typedefs can stay the same as in the UFS to facilitate future updates back and forth. The original GFDL_parse_tracers.F90 hasn't change since the beginning of time, and therefore I do not anticipate changes to the NRL version unless we start using a new tracer in NEPTUNE with a different name than the FV3 name.

Note. Because the NRL fork is behind NCAR main, this PR currently adds the file in the original location of GFDL_parse_tracers.F90 - in physics. I was hoping to move it to the new location of GFDL_parse_tracers.F90, but then I realized this file is now in physics/MP/GFDL - probably not the best place. This file has nothing to do with GFDL microphysics. It's a "hook" or a "tool" that is used to parse tracer names from the FV3 dycore (and by extent the UFS) in an array, entirely independent of the choice of microphysics. Therefore, I suggest that I move the NRL version to either hooks or tools in this PR (whatever @dustinswales and @grantfirl think is best) and then create an issue that someone moves GFDL_parse_tracers.F90 to the same location (this will require metadata changes, for example in GFS_typedefs.meta, where it is specified as a dependency).

@dustinswales
Copy link
Collaborator

@climbfuji I can't recall why I put the GFS_parse_tracers file in the MP/GFDL subdirectory. Most likely because of the name :)

But yeah, it doesn't make sense to have it there. Actually, I don't this code belongs in the physics at all, and should probably live with the host (in its typedefs). For the UFS, we could put the get_tracer_index() function here, and somewhere analogous for NEPTUNE?

@climbfuji
Copy link
Collaborator Author

Do you recall why it's been added to ccpp-physics at all?

@dustinswales
Copy link
Collaborator

I have no insight on its history.
Maybe @grantfirl does?

@grantfirl
Copy link
Collaborator

@dustinswales @climbfuji I don't understand why this is in the physics at all. It should be part of the host model. This goes for both GFDL_parse_tracers and NRL_parse_tracers. The fact that it was seen fit to make an NRL-specific version tells us that this should probably be in the host too. The parse_tracers module is only referenced by host model code.

@dustinswales
Copy link
Collaborator

@climbfuji @grantfirl
Alright then, so as part of an upcoming set of fv3atm/ccpp-physics PRs we should move the GFS_parse_tracers.F90 file, or just its contents, from the physics to the host (e.g. directly to GFS_typedefs.F90 or move the file to that level?)
Dom, then you keep the NRL version under the NEPTUNE hosts control?

@climbfuji
Copy link
Collaborator Author

@climbfuji @grantfirl Alright then, so as part of an upcoming set of fv3atm/ccpp-physics PRs we should move the GFS_parse_tracers.F90 file, or just its contents, from the physics to the host (e.g. directly to GFS_typedefs.F90 or move the file to that level?) Dom, then you keep the NRL version under the NEPTUNE hosts control?

Ok. NEPTUNE is trying to follow the UFS directory structure for CCPP. How about we move the file into FV3/ccpp/data in the UFS, and similar for NEPTUNE?

@climbfuji
Copy link
Collaborator Author

I'll convert this back to a draft PR but keep it open until we've made the changes we discussed.

@climbfuji climbfuji marked this pull request as draft September 19, 2024 17:00
@climbfuji
Copy link
Collaborator Author

A PR was created in the NRL enterprise github to remove both files, GFDL_parse_tracers.F90 and NRL_parse_tracers.F90. I will update this PR to remove both files in NCAR ccpp-physics main, too.

…ers.F90; update CODEOWNERS physics/docs/ccpp_doxyfile
@climbfuji climbfuji changed the title Add NRL_parse_tracers.F90 as a replacement for GFDL_parse_tracers.F90 for NEPTUNE ~~Add NRL_parse_tracers.F90 as a replacement for GFDL_parse_tracers.F90 for NEPTUNE~~ Remove GFDL_parse_tracers.F90 Sep 25, 2024
@climbfuji climbfuji marked this pull request as ready for review September 26, 2024 02:33
@climbfuji
Copy link
Collaborator Author

I've opened this PR up for reviews again. I did not create the necessary PR in fv3atm for it, and it's not entirely clear to me how we will do this - this PR is for NCAR ccpp-physics main, not for the ufs/dev branch in the ufs-community fork. Please advise.

@dustinswales
Copy link
Collaborator

I've opened this PR up for reviews again. I did not create the necessary PR in fv3atm for it, and it's not entirely clear to me how we will do this - this PR is for NCAR ccpp-physics main, not for the ufs/dev branch in the ufs-community fork. Please advise.

@climbfuji Thanks for modifying the PR to move this file to the host's control.
Since this is into NCAR:main, we need to open a PR to the SCM adding GFDL_parse_tracers to that repo. Then @grantfirl will open a PR into ufs-community:ufs/dev from NCAR:main.
The SCM RTs are all CI based and will get launched when the PR is created, so this part could be ready quickly. However, there is also the added burden of "running all participating hosts RTs before something can get merged into the root", so Grant (or someone else?) has to run the UFS RTs using NCAR:main.
Yuck.

@dustinswales
Copy link
Collaborator

@climbfuji I created a PR into the SCM with this change. See NCAR/ccpp-scm#520

@climbfuji
Copy link
Collaborator Author

@dustinswales @grantfirl We merged the corresponding PR in the NRL fork that removes both NRL_parse_tracers.F90 and GFDL_parse_tracers.F90

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