Remove 'goto' statements from chgres_cube

[GeorgeGayno-NOAA]

NCO is becoming more strict with their coding standards, especially in regards to 'goto' statements. The HAFS group wants to use the v1.9.0 release in their upcoming implementation (code delivery date is March 31, 2023). But they are concerned that NCO will reject their implementation package because of the 'goto' statements in chgres_cube.

The Whole Atmosphere Model (WAM) option of chgres_cube contains >99% of the 'goto' statements. Some of these WAM functions can be easily converted. However, other WAM functions use convoluted logic that won't be simple to fix. For example, here is a 'goto' from function gts7.

17 continue 
      go to (20,50,20,25,90,35,40,45,25,48,46),  j 
20 continue 

The HAFS group uses chgres_cube to create lateral boundary conditions. They do not use the WAM option.

So, what to do?

• Ask the WAM group if they even use the WAM option. If not, remove it from chgres_cube.
• If the WAM group does use it, task them to fix these routines (and write unit tests).
• If the fix will take several weeks, then temporarily remove the WAM option and cut a new release for HAFS. The WAM code is very modular, so it can be easily removed, then added back in later if necessary.

[GeorgeGayno-NOAA]

I created a branch where I removed the WAM option. See 63f29d6.

[GeorgeGayno-NOAA]

@akubaryk Has a version with no 'goto' statements. See his fork at 032dff6.

[GeorgeGayno-NOAA]

@akubaryk I have looking through Henry's version of "wam_climo_data.f90". I see references to 'MSIS', defined as Mass Spectrometer and Incoherent Scatter radar. In your branch, I also see references to 'MSIS'. Did you simply replace Henry's old functions with newer versions from NRL?

[akubaryk]

Correct, the primary change here is replacing NRLMSISE-00 with NRLMSIS 2.1, which is a welcome improvement in initializing the upper atmosphere. The NRLMSIS 2.1 source also does not contain any goto statements that I'm aware of.

My branch has a major issue in that it retrieves the MSIS 2.1 source from NRL rather than including it in the branch, which would cause the build to fail on e.g. Hera. NRL's license with MSIS 2.0 was extremely restrictive, but we may be able to use the 2.1 source without issue (with some kind of included statement about the use of MSIS and its license); I haven't had a chance to read the new license closely, though.

I also haven't tested it at all beyond checking that it compiles cleanly on WCOSS2.

[GeorgeGayno-NOAA]

@akubaryk I see your branch downloads a zip file from an NRL web site. That won't work well. How many MSIS routines do you use? Can we host them as part of UFS_UTILS? Are these routines on Github somewhere so we can get them as submodule?

[akubaryk]

NRL does not make MSIS available on GitHub. The license reads as such:

MSIS� (NRL-SOF-014-1) SOFTWARE
OPEN SOURCE ACADEMIC RESEARCH LICENSE AGREEMENT

1. Agreement. The MSIS� empirical atmospheric model software (hereinafter
�Software�) is property of The Government of the United States of America. This
software is being made available under the following terms and conditions. By
using, modifying, reproducing, or preparing a derivative work of this Software,
you agree to abide by the terms and conditions herein.

2. License. In accordance with federal law, authorization is given to use,
reproduce, and modify the Software solely for research, academic, and non-profit
purposes and only in accordance with the terms and conditions in this Agreement.
Any commercial use is prohibited. No other rights or permissions are provided.    

3. Basis. The Software was written by employees of the U.S. Naval Research
Laboratory (NRL) and is property of the United States Government, as represented
by the Secretary of the Navy. MSIS� is a registered trademark of the Government
of the United States of America, as represented by the Secretary of the Navy.
Unauthorized use of the trademark is prohibited. 

4. Restrictions and Use. 

a. Sales. A user of the Software shall not sell, or license, or transfer for a
fee the Software or portion thereof, or any derivative work of the Software, or
any data products generated by the Software, without first obtaining the written
consent of IP Counsel for the Naval Research Laboratory.

b. Modifications.  All modifications to the Software and derivative works of the
Software (including translations to other programming languages) shall carry
prominent notices stating how the files were changed and the date of the change.
Any party who modifies the Software shall deliver the modified portion of the
Software to authors or NRL Code 7630, U.S. Naval Research Laboratory. Any
reproductions, modified versions of the Software, or derivative works shall be
made available to the public as open source software. A party who modifies the
Software or creates a derivative work of the Software hereby grants the
Government of the United States of America a non-exclusive, irrevocable, fully
paid-up license to such modifications and derivative works, and shall deliver
such derivative works, including any source code, data, or information that
pertains to such derivatives works, to Code 7630, U.S. Naval Research
Laboratory. If any modifications to the Software substantially change the model
output (including, but not limited to, alterations of the model formulation or
model parameter values), then the modified Software shall not be identified
�MSIS� without first obtaining the written consent of Counsel, Office of Naval
Research, Department of the Navy. In such cases, the MSIS acronym shall
nonetheless still appear in the individual files in order to document the
provenance of the modified Software.

c. Notices.  Each copy of the Software, any modified Software, or derivative
work shall include a file containing this Agreement. Any software package that
incorporates the MSIS� Software or derivative work shall include the following
statement: "This software incorporates the MSIS� empirical atmospheric model
software designed and provided by NRL. Use is governed by the Open Source
Academic Research License Agreement contained in the file
nrlmsis2.1_license.txt."

5. Disclaimer of Warranty and Liability: As the owner of the MSIS� software, the
Government of the United States of America: (1) Disclaims any warranties,
express, or implied, including but not limited to any implied warranties of
merchantability, fitness for a particular purpose, title or non-infringement,
(2) Does not assume any legal liability or responsibility for the accuracy,
completeness, or usefulness of the software, (3) Does not represent that use of
the software would not infringe privately owned rights, (4) Does not warrant
that the software will function uninterrupted, that is error-free or that any 
errors will be corrected.

6. No Support.  The Software is provided without any support or maintenance, and
without any obligation to provide modifications, improvements, enhancements, or
updates of the Software. No oral or written information or advice given by the
NRL authors shall create a warranty or in any way modify this agreement. Should
the Software prove defective, the user (and not NRL or any NRL representative)
assume the cost of all necessary correction.

Without having read it closely, I don't know if we can host the source as a part of UFS_UTILS. Seems like 4c might allow it if we follow the instructions, but I don't know if that's compatible with the standard UFS LGPLv3.0 license

[GeorgeGayno-NOAA]

@akubaryk I just want to verify that your group is not using the WAM function as it is currently implemented in the authoritative fork. Is that correct?

If you are not using it, would you mind if I remove that functionality from the authoritative fork? You can always add it back using the desired MSIS routines in the future.

If you think you can update chgres_cube using the MSIS routines within the next two weeks or so, I will hold off. But the HAFS group needs a tag for an upcoming implementation. They don't use the WAM functionality. And they need a tag fairly quickly.

[akubaryk]

@GeorgeGayno-NOAA we would like to keep the code in the repository. What needs to be resolved in my branch to accomplish this?

[GeorgeGayno-NOAA]

@GeorgeGayno-NOAA we would like to keep the code in the repository. What needs to be resolved in my branch to accomplish this?

Can you eliminate the download of the zip file? Can the required routines be hosted under UFS_UTILS? How many routines are there?

[akubaryk]

Who can we talk to about the license issue? We need most of the MSIS package, and it all falls under the license I pasted above.

[GeorgeGayno-NOAA]

Who can we talk to about the license issue? We need most of the MSIS package, and it all falls under the license I pasted above.

It sounds like we can use the code as long as we display the license. Perhaps we can keep the MSIS code in a separate directory, compile it as a library (using the ufs_utils cmake build), then map it into chgres_cube.

@edwardhartnett Does NCEPLIBS use any code with a similar worded license?

[arunchawla-NOAA]

@akubaryk and @GeorgeGayno-NOAA

Let me know if you need my help on the license issue

[edwardhartnett]

How about we put the license in the comments at the top of the code file and go wiht that?

[GeorgeGayno-NOAA]

@akubaryk While we discuss the details of the license, please go ahead and come up with a way to implement these routines as part of UFS_UTILS.

[akubaryk]

4ed6de8 contains the MSIS 2.1 source in a subdirectory of chgres_cube.fd, updates the cmake tree appropriately, and includes the license notice as required by the MSIS 2.1 license at the top of wam_climo_data.f90.

[GeorgeGayno-NOAA]

@akubaryk are your changes ready to be merged? If so, please create a PR.

[GeorgeGayno-NOAA]

@akubaryk I made a copy of your branch in my fork. I fixed a problem with the read of the msis parm file. See 865bb17. But when I try to run the WAM-based regression test, I get this error:

 VINTG_WAM:- CALL FieldGet FOR 3-D TRACER spo2
 VINTG_WAM:- CALL FieldGet FOR 3-D TRACER graupel
 VINTG_WAM:- CALL FieldGet FOR 3-D TRACER spo
Error in pwmp
Error in pwmp
Error in pwmp
Error in pwmp
Error in pwmp

To run the test on Hera, I used this script: /scratch1/NCEPDEV/da/George.Gayno/ufs_utils.git/UFS_UTILS/reg_tests/chgres_cube/driver.hera.sh.4.adam

[akubaryk]

Thanks, @GeorgeGayno-NOAA -- returning from leave today, will look at this as soon as I am able.

[GeorgeGayno-NOAA]

Where did you run the test? I want to check the log file.

…

On Wed, Mar 1, 2023 at 4:04 PM akubaryk ***@***.***> wrote: Does this work now? I had no issues running the regression test when copying /scratch1/NCEPDEV/da/George.Gayno/ufs_utils.git/UFS_UTILS into my own space. If so -- thank you for your efforts bringing my branch up to speed in my absence! — Reply to this email directly, view it on GitHub <#759 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMSYBTBGDYCHUGCC4WXSBXLWZ62XDANCNFSM6AAAAAAUAR3FDI> . You are receiving this because you were mentioned.

[akubaryk]

Sorry, that was sent in error. The commit I was using was not the one referenced in the issue.

[akubaryk]

Appear to be some type errors in wam_climo_data.F90 that I think I have resolved correctly, check the log here: /scratch1/NCEPDEV/swpc/Adam.Kubaryk/UFS_UTILS.gg/reg_tests/chgres_cube/consistency.log15

Of course fails the regression test itself because MSIS 2.1 is not MSIS 1.0, but that's fine. I can have someone in my group look at the output for a science review if we think that's useful. (Is there an easy way to regrid the individual tiles to get a convenient global view? I think I remember some such software that was able to do it on the fly but cannot recall now.)

I will update the PR if all looks good.

[GeorgeGayno-NOAA]

Appear to be some type errors in wam_climo_data.F90 that I think I have resolved correctly, check the log here: /scratch1/NCEPDEV/swpc/Adam.Kubaryk/UFS_UTILS.gg/reg_tests/chgres_cube/consistency.log15

Of course fails the regression test itself because MSIS 2.1 is not MSIS 1.0, but that's fine. I can have someone in my group look at the output for a science review if we think that's useful. (Is there an easy way to regrid the individual tiles to get a convenient global view? I think I remember some such software that was able to do it on the fly but cannot recall now.)

I will update the PR if all looks good.

Just checked your changes into my branch (ff87f3c). The regression test ran. You will need to check the results. Yes, a science review is a good idea. And I would run some coldstart files though the forecast model. @DusanJovic-NOAA has a tool that pieces together the six tiles for visualization.

[akubaryk]

This is one that George will likely need to help us with, but there are some options in /scratch1/NCEPDEV/nems/role.ufsutils/ufs_utils/reg_tests/chgres_cube/baseline_data -- directories c96_fv3_netcdf.* have out.sfc.tile?.nc files.

[GeorgeGayno-NOAA]

This is one that George will likely need to help us with, but there are some options in /scratch1/NCEPDEV/nems/role.ufsutils/ufs_utils/reg_tests/chgres_cube/baseline_data -- directories c96_fv3_netcdf.* have out.sfc.tile?.nc files.

The last step compares the out.sfc.tile?.nc files from the test (see your working directory) to those from the baseline. If they were identical, you can use either set. Otherwise, use the ones from the working directory.

[akubaryk]

We don't have out.sfc.tile?.nc files in our existing regression test, but I have just added export SFC_FILES_INPUT=gfs.t00z.sfcf000.nc below the ATM_FILES_INPUT assignment in c96.fv3.netcdf2wam.sh and dropped the CONVERT_???=".false." lines to get the expected surface outputs -- @SvetlanaKarol-NOAA, check /scratch2/NCEPDEV/stmp1/Adam.Kubaryk/reg-tests/chgres-cube/c96_fv3_netcdf2wam again for those files.

@GeorgeGayno-NOAA should we also include those changes into our regression test in this PR?

[SvetlanaKarol-NOAA]

The temperature, winds, tracers from 5days FV3WAM run output with ICs created from FV3->FV3WAM chgres applications look good. One issue: the global temperature near the mesopause( top lid of FV3GFS-128L) is getting colder during the run.

[GeorgeGayno-NOAA]

We don't have out.sfc.tile?.nc files in our existing regression test, but I have just added export SFC_FILES_INPUT=gfs.t00z.sfcf000.nc below the ATM_FILES_INPUT assignment in c96.fv3.netcdf2wam.sh and dropped the CONVERT_???=".false." lines to get the expected surface outputs -- @SvetlanaKarol-NOAA, check /scratch2/NCEPDEV/stmp1/Adam.Kubaryk/reg-tests/chgres-cube/c96_fv3_netcdf2wam again for those files.

@GeorgeGayno-NOAA should we also include those changes into our regression test in this PR?

You are right. The WAM regression test only outputs the 'atm' files. Since the WAM function only concerns the atmosphere, there is no need to update the test to process the surface fields.

[akubaryk]

The temperature, winds, tracers from 5days FV3WAM run output with ICs created from FV3->FV3WAM chgres applications look good. One issue: the global temperature near the mesopause( top lid of FV3GFS-128L) is getting colder during the run.

Thanks, @SvetlanaKarol-NOAA!

Are we good to go with the PR -- do I need to do anything else from here @GeorgeGayno-NOAA?

[GeorgeGayno-NOAA]

I can create a PR. Just to confirm, you tested with my fork at ff87f3c. You did not have local changes, correct?

[SvetlanaKarol-NOAA]

Yes, I think we are good, we could figure out the issue with temperature later.

[akubaryk]

I can create a PR. Just to confirm, you tested with my fork at ff87f3c. You did not have local changes, correct?

Yes, the only changes I made were to the regression test scripts for various matters of convenience, no source changes to any part of chgres_cube.

[GeorgeGayno-NOAA]

I can create a PR. Just to confirm, you tested with my fork at ff87f3c. You did not have local changes, correct?

Yes, the only changes I made were to the regression test scripts for various matters of convenience, no source changes to any part of chgres_cube.

Ok. You will need to add my changes to your fork or close your PR and I can submit a PR from my fork. I would prefer the former because I am an official reviewer for the repo.

[GeorgeGayno-NOAA]

@akubaryk I made some mostly minor updates.

I see that the library came with its own test. I incorporated this test under Github actions with our other unit tests. It reads the file provided with the library, computes some fields, then compares the computed values with the 'check' values. The computed and check values will never be exact. So, I guessed at a threshold at which the test fails. Your comments are welcome.

If you merge my changes to your branch, you can start a PR.

[akubaryk]

#775 has been updated with the proposed minor changes.

I wish I had more insight on the MSIS tests, but unfortunately I do not. As long as it doesn't fail right now, it's probably fine overall.