Clean up surface.F90 and simplify nst_land_fill with FieldBundle loop (this one will be correct, I swear) #519
Conversation
|
@LarissaReames-NOAA Why create the field bundle in 'nst_land_fill'? Why not do that earlier in say 'create_nst_esmf_fields'. That way the bundle can be used in other places. For example, in routine 'interp' a bundle regrid could be done to shorten the code by eliminating all those individual regrids. |
The fieldbundle used here actually doesn't include all of the nst fields as there are two (tref and xz) which need to be set to different values then the others which get set to 0. It was either that, or include those two in the bundle write up an if statement that checked for the name of the field and did different things based on that. I figure it's probably close to a wash in code length difference between those two approaches. I actually have the latter coded up in a different branch, but I'm waiting for the soiltype fix to be merged in before submitting that PR. In that branch, I use bundles for each type of masked regridding and also use those same bundles to loop over for the search function as well. In the water regrid section, I include nst fields with the bundle if the nst flag is set, not in a separate bundle. I was able to cut out close to 1000 lines of code by using these bundles throughout the interp routine, even with the addition of the two subroutines I wrote to regrid and search over the bundles. In total, I'm not sure creating a bundle with only nst fields and reusing this field bundle in two places would save that much code since there aren't any other places where only nst fields are looped over. Since the water regrid includes other fields, I'd have to create a bundle with only those non-nst fields and call the regrid and search routines twice in that case. Does any of this makes sense? If you want you can look at my other branch (feature/sfc_regrid_many, no PR yet) to see what my code looks like. |
sorc/chgres_cube.fd/surface.F90
Outdated
| farrayPtr=data_ptr, rc=rc) | ||
| if(ESMF_logFoundError(rcToCheck=rc,msg=ESMF_LOGERR_PASSTHRU,line=__LINE__,file=__FILE__)) & | ||
| call error_handler("IN FieldGet", rc) | ||
| do i = 1,16 |
There was a problem hiding this comment.
There is an ESMF routine that will return the number of fields (fieldCount) within a bundle:
http://earthsystemmodeling.org/docs/release/ESMF_8_1_1/ESMF_refdoc/node5.html#SECTION050251000000000000000
There was a problem hiding this comment.
Yeah, I use that functionality in the new *****_many routines in the sfc_regrid_many branch, but in this case I know the field number exactly so I didn't really see the benefit to calling another ESMF routine in this case.
There was a problem hiding this comment.
In that case please define a parameter NUM_ESMF_FIELDS or something of that sort. You should not use bare constants in production code (it's OK to do so in test code).
There was a problem hiding this comment.
Done. I also converted the other bare constants that had been used in the code prior to my changes to parameters as well.
|
Did you run the consistency tests? I can do so on the machines you can't access. |
|
@GeorgeGayno-NOAA They passed on Hera. |
Merge the latest updates from 'develop'. Then I can run them today and, hopefully, merge. |
…feature/sfc_cleanup
|
The branch has been updated and passes tests. |
|
So it looks like. while the consistency tests currently in the develop branch all pass, the tests I'd written for the regional capabilities (PR #252) are now broken. It appears this was caused by the implementation of the new tracers in PR #388 as these tests passed before I merged in the most recent develop branch changes |
Tests passed on Hera, Orion, Jet, WCOSS-Cray and WCOSS-Dell |
|
All develop branch and regional grib2 system/consistency tests now pass on Hera after implementing a few fixes for the new num_tracers_input usage. Unit tests also pass. |
I had no problems running the consistency tests for this PR. Which tests failed for you? |
The consistency tests that have been in a PR to the release branch for months and are just as suitable for the develop branch. They test a much wider range of capabilities that the code has for grib2 files, including reading in the Thompson aerosol variables. I've been using them to test as I develop for the develop branch. I'm going to submit a PR later today to the develop branch so we can get those in. |
|
@LarissaReames-NOAA Is this ready to merge? |
|
@GeorgeGayno-NOAA Yes, the tracer issues were fixed and I updated the test for reading the variable mapping table to check for the new num_tracers_input variable. |
This PR addresses Issue #516
I removed some old code that's no longer used and converted nst_land_fill to using a FieldBundle loop to cut down on code length. I also wrote a test for this subroutine that checks that land points in all of the various nst fields are being set properly. All tests pass as do the old system tests.
Third time's a charm