Clean up sfcsub.F: remove unused code, comments, and developer tags

[ClaraDraper-NOAA]

As a first step to cleaning up the sfcsub routine, this PR removes obvious unused code and excessive commenting. It reduces the length of the code substantially, making it a little easier to parse/work with.

These edits were made by by co-pilot, with my reviewing them and adding some comments back in.

These changes should not affect the functioning of the code in any way.

Description of Changes:

Specific changes reported by co-pilot.

• Removed ~700 lines of excessive comments
• Removed 790 lines of isolated comment markers and excessive blank lines
• Removed 3 unused epsilon variables (epsoro, epsplr, epsacn)
• Removed 2 unused namelist variables (qcmsk, znlst)
• Removed 3 unused subroutines (count, dayoyr, maxmin)
• Removed 31 lines with developer suffix comments (cbosu, cggg, landice, cjfe)
• Removed all inline comments from routine argument lines
• Removed 7 commented-out old argument lines
• Removed 9 lines of commented-out old code (print statements and calculations)

Total reduction: ~1600 lines from original 8892 to 7275 lines

Tests Conducted:

on gaea:
Ran 4 cycles of the C96C48_hybatmDA global_workflow test case, confirmed that results are zero-diff. This will test the routine as called by both the model and UFS_UTILS.

All ufs-weather-model tests passed on URSA.

on ursa:
all UFS_UTILS/global_cycle tests passed.

Dependencies:

N/A

Documentation:

N/A

Issue (optional):

First step to addressing issue #344

[ClaraDraper-NOAA]

draft while I do testing.

[ClaraDraper-NOAA]

@XuLi-NOAA Could you please review this PR?

I think the real issue is to make sure the gcycle is called at the initial time of the IAU, where the model forecast is involved. Then, to determine how often the gcycle will be called in the forecast mode. I think nscyc, a namelist parameter for model should be set as intended, probably larger than 6 hours (the IAU time window), which means gcycle is called only once (at the initial time). Importantly, we need to figure out why gcycle is called twice in IAU, which is supposedly unexpected.
In summary, the content of this PR needs to be modified to address two issues: 1. Why gcycle is called twice? 2. Make sure gcycle is called at the initial time, not at other times. In other words, gcycle should be called only one time and at the initial time, unless it is designed so.

[ClaraDraper-NOAA]

Tests have passed, this is ready to review.

[ClaraDraper-NOAA]

@XuLi-NOAA Could you please review this PR?

I think the real issue is to make sure the gcycle is called at the initial time of the IAU, where the model forecast is involved. Then, to determine how often the gcycle will be called in the forecast mode. I think nscyc, a namelist parameter for model should be set as intended, probably larger than 6 hours (the IAU time window), which means gcycle is called only once (at the initial time). Importantly, we need to figure out why gcycle is called twice in IAU, which is supposedly unexpected. In summary, the content of this PR needs to be modified to address two issues: 1. Why gcycle is called twice? 2. Make sure gcycle is called at the initial time, not at other times. In other words, gcycle should be called only one time and at the initial time, unless it is designed so.

@XuLi-NOAA I don't see the relevance of the timing of the global_cycle call to this PR. Does this comment relate to this PR instead? Note that that PR has been approved and merged. No further work is required on it (but to respond to your comment: with IAU on, global_cycle is called on the sfc_data files once at the start of the DA window ( to be used to start the model) and again in the middle of the DA window. My understanding is that the output of the latter is needed by products. It is called twice, this is correct / intended.

[XuLi-NOAA]

@XuLi-NOAA Could you please review this PR?
I think the real issue is to make sure the gcycle is called at the initial time of the IAU, where the model forecast is involved. Then, to determine how often the gcycle will be called in the forecast mode. I think nscyc, a namelist parameter for model should be set as intended, probably larger than 6 hours (the IAU time window), which means gcycle is called only once (at the initial time). Importantly, we need to figure out why gcycle is called twice in IAU, which is supposedly unexpected. In summary, the content of this PR needs to be modified to address two issues: 1. Why gcycle is called twice? 2. Make sure gcycle is called at the initial time, not at other times. In other words, gcycle should be called only one time and at the initial time, unless it is designed so.

@XuLi-NOAA I don't see the relevance of the timing of the global_cycle call to this PR. Does this comment relate to this PR instead? Note that that PR has been approved and merged. No further work is required on it (but to respond to your comment: with IAU on, global_cycle is called on the sfc_data files once at the start of the DA window ( to be used to start the model) and again in the middle of the DA window. My understanding is that the output of the latter is needed by products. It is called twice, this is correct / intended.

In principle, gcycle is called during a forecast mode, at the initial time 100% and how often for later on depends on the setting of one model namelist paramter. I understand IAU works with a model integration, even if itself is an analysis issue, and the available analysis increment is applied to the model integration when needed (I guess, in 6-hour IAU time window, this happens 6 times or houly). Therefore, the timing of gcycle is relavent, I understand.

[ClaraDraper-NOAA]

@BrianCurtis-NOAA Thanks for looking at this. I think I've addressed everything.

[grantfirl]

@ClaraDraper-NOAA Do you plan to open supermodule PRs and run UFS regression tests on this?

[ClaraDraper-NOAA]

@ClaraDraper-NOAA Do you plan to open supermodule PRs and run UFS regression tests on this?

@grantfirl Thanks for looking at this.

Looping in @BrianCurtis-NOAA, as the sfcsub routine is also called by UFS_UTILS/global_cycle

Grant - this PR is zero diff for the output of sfscub (essentially it removes excessive comments). I have at least one more coming that will also be zero diff (removing code that cannot be exercised / is incomplete / very out-dated), and another that will be zero diff for the inputs that we use from ufs_model and UFS_UTILS (removing code that we are not currently exercising; simplifying code). I have two questions:

1. How would your team like me to handle the supermodule PRs? Eventually there will need to be updates to the two calling routines (in UFS_UTILS and ccpp-physics), and associated ufs_model supermodules, and also to the global_cycle and ufs_utils scripts that prepare the namsfc namelist used by sfcsub.

My suggestion is that I construct all of the above PRs such that they do not affect the calling programs in any way, so that we do not need to do the supermodule updates. I have started collecting the namelist and argument changes (all deletions), and can then submit a fourth PR after the above three are merged that consists only of removing the namelist and argument variables from throughout the various modules / scripts.

2. How would you like me to handle testing? My suggestion is that I write a separate wrapper to test that my changes to sfcsub do not change any output for the range of possible functionality that we are using in either ufs_model or ufs_utils (as in it will explicitly test for changes in output from the different combinations of input variables). I will also do the standard UFS_UTILS/global_cycle tests (these check for zero diff change), and check two of the global_workflow tests are zero diff after cycling through one day (one test mimics GFSv17 and one mimics the uncoupled atmos model).

[grantfirl]

@ClaraDraper-NOAA Do you plan to open supermodule PRs and run UFS regression tests on this?

@grantfirl Thanks for looking at this.

Looping in @BrianCurtis-NOAA, as the sfcsub routine is also called by UFS_UTILS/global_cycle

Grant - this PR is zero diff for the output of sfscub (essentially it removes excessive comments). I have at least one more coming that will also be zero diff (removing code that cannot be exercised / is incomplete / very out-dated), and another that will be zero diff for the inputs that we use from ufs_model and UFS_UTILS (removing code that we are not currently exercising; simplifying code). I have two questions:

1. How would your team like me to handle the supermodule PRs? Eventually there will need to be updates to the two calling routines (in UFS_UTILS and ccpp-physics), and associated ufs_model supermodules, and also to the global_cycle and ufs_utils scripts that prepare the namsfc namelist used by sfcsub.

My suggestion is that I construct all of the above PRs such that they do not affect the calling programs in any way, so that we do not need to do the supermodule updates. I have started collecting the namelist and argument changes (all deletions), and can then submit a fourth PR after the above three are merged that consists only of removing the namelist and argument variables from throughout the various modules / scripts.

2. How would you like me to handle testing? My suggestion is that I write a separate wrapper to test that my changes to sfcsub do not change any output for the range of possible functionality that we are using in either ufs_model or ufs_utils (as in it will explicitly test for changes in output from the different combinations of input variables). I will also do the standard UFS_UTILS/global_cycle tests (these check for zero diff change), and check two of the global_workflow tests are zero diff after cycling through one day (one test mimics GFSv17 and one mimics the uncoupled atmos model).

What is the reason for separating these 3 pieces of work into separate PRs? To make it easier to review due to the large number of changes?

If so, since this one has already been reviewed, perhaps you could add on to it and reviewers could review the next separately when it is ready? It is easy enough to review individual commits that come in if we want to avoid sifting through the already-completed changes.

I suppose that I would hold off on creating supermodule PRs in UFSATM, ufs-weather-model, and UFS-utils until all of the work with sfcsub.F is completed. Supermodule PRs are needed regardless of whether any code is changed within them or not. At the very least, they will just have submodule commit hash changes (and changes to .gitmodules for the automatic testing).

Your testing plan sounds great, but we'll also have to run UFS regression tests on multiple platforms in order to merge it once you're satisfied with your own individual testing.

[ClaraDraper-NOAA]

Yes, I'm separating them as each will be rather large, and do slightly different things. I like your idea of adding everything into this PR. Then I'll do the full regression tests once everything is finalized.

[ClaraDraper-NOAA]

All ufs-weather-model tests have passed on URSA. Super module PRs submitted:

• UFS WEATHER MODEL PR

• UFSATM: PR

This is ready to be merged.

[rhaesung]

@ClaraDraper-NOAA Thanks for submitting these super module PRs and for confirming that all ufs-weather-model tests passed on URSA.
For both PRs, please update the .gitmodules files to point to your working branches. For the UFS PR, also update the test_changes.list and URSA log files so the UFS code managers can refer to the results. Once these updates are made, the PRs will be ready to enter the commit queue.

[gspetro-NOAA]

Testing has completed successfully on WM PR 3072. This PR can be merged.

[ClaraDraper-NOAA]

@ClaraDraper-NOAA Thanks for submitting these super module PRs and for confirming that all ufs-weather-model tests passed on URSA. For both PRs, please update the .gitmodules files to point to your working branches. For the UFS PR, also update the test_changes.list and URSA log files so the UFS code managers can refer to the results. Once these updates are made, the PRs will be ready to enter the commit queue.

@rhaesung As you probably realized, I was on leave yesterday. It looks like you've already sorted all of this out - thank you! Please let me know if you need anything else from me (note - I'm on leave again from tomorrow for 10 days).