GEOS-ESM / NCEP_Shared

Apache License 2.0
2 stars 1 forks source link

If we use DYNAMIC_ALLOCATION in BUFRLIB is it zero-diff? #17

Open mathomp4 opened 3 years ago

mathomp4 commented 3 years ago

Pursuant to #16, a workaround had to be added to let BUFRLIB build with GCC. The issue is that the current compile strategy for GEOS is to not define -DDYNAMIC_ALLOCATION so it proceed with 'static' I guess. The question is: if we use DYNAMIC_ALLOCATION for Intel as well, do we get the same answer.

An issue was opened with NCEP: https://github.com/NOAA-EMC/NCEPLIBS-bufr/issues/81 and another issue was linked to by @edwardhartnett about moving to dynamic by default: https://github.com/NOAA-EMC/NCEPLIBS-bufr/issues/77 As NCEP might move to DA by default (and I suppose only to dynamic), it would behoove us to test this.

The issue is: I have no idea how to test NCEP_bufr. I know how to do the CMake, but beyond that... 🤷‍♂️

I'm going to assign both myself and @gmao-msienkie only because she is who I think of with "bufr". But if she has a better person in mind, I'll ping/assign them. Once the GEOSadas is (officially) in Git, we can work on this. (though I could also tell someone how to do the same thing in CVS/GNU make)

gmao-msienkie commented 3 years ago

I don't think that there was any difference in the results when compiling with and without -DDYNAMIC_ALLOCATION, in part because the code runs essentially the same if none of the default settings are changed. The dynamic allocation allows increasing the maximum sizes of some of the arrays used in the library, and allowed NCEP to retire their 'supersize' build.

I know that we had deliberately chosen to build without dynamic allocation. There may have been some issue with performance but the real issue was that when I tested running the GSI with the library built with dynamic allocation, back in May 2017, and the program crashed in the 'read_iasi.f90' routine. From what I could see, the BUFRLIB code sets up things to use the dynamic allocation the first time there is a call to OPENBF. However, in 'read_iasi.f90' there is a call to CLOSBF before the first call to OPENBF. The CLOSBF call does nothing because there is no BUFR file open but it runs afoul of the dynamic allocation stuff because it hasn't been set up by the first OPENBF call. I considered trying to put something at the beginning of the GSI to make sure the stuff for dynamic allocation was intialized before anything was done with any BUFR files - in case there were other cases in the code with a similar problem. However at the time I decided that we would just stay with our 'static allocation' the way that we had always done it and avoid the issue of having to make it work throughout the code... because the GSI didn't need it and I think there also was mention of a potential performance hit with the dynamic allocation. (Though of course because of the other issue we did not persue this.) If you want to pass this information back to NCEP you are welcome.

mathomp4 commented 3 years ago

@gmao-msienkie Thanks. I'll pass it along. That said, it looks like the develop branch of NCEPLIBS-bufr does have newer code in https://github.com/NOAA-EMC/NCEPLIBS-bufr/blob/develop/src/closbf.F where at the top:

#ifdef DYNAMIC_ALLOCATION
      IF ( .NOT. ALLOCATED(NULL) ) THEN
        CALL ERRWRT('++++++++++++++++++++WARNING++++++++++++++++++++++')
        ERRSTR = 'BUFRLIB: CLOSBF WAS CALLED WITHOUT HAVING ' //
     .           'PREVIOUSLY CALLED OPENBF'
        CALL ERRWRT(ERRSTR)
        CALL ERRWRT('++++++++++++++++++++WARNING++++++++++++++++++++++')
        RETURN
      ENDIF
#endif

Perhaps when the develop branch is merged into a new release we might be able to explore this?

mathomp4 commented 3 years ago

Actually, it looks like BUFR 11.3.2 might have the code as well:

https://github.com/NOAA-EMC/NCEPLIBS-bufr/blob/bufr_v11.3.2/src/closbf.F

mathomp4 commented 3 years ago

And the ReleaseNotes note this: https://github.com/NOAA-EMC/NCEPLIBS-bufr/blob/develop/docs/ReleaseNotes.md#version-1132---july-16-2020

gmao-msienkie commented 3 years ago

Getting back to your original comment about 'how to test NCEP_bufr'. In the ADAS I believe that the BUFR library is used primarily in running the GSI analysis and in the various preprocessing QC and data manipulation programs under Applications/NCEP_Paqc. Oh, and of course the EnKF component. I would usually run some single analysis cases of the GSI to make sure it is working as expected - almost always zero diff. (I suppose one of Joe's DAS tests could be used for that purpose.) The NCEP_Paqc programs both read and write BUFR files. I had a script to run the 'gmao_prepqc' by itself (probably needs updating) or I use a 'prepqc.yyyymmdd.hh' working directory from a previous run and try and reproduce the results. If the BUFR version changes you won't be able to get a zero diff out of 'cmp' but I have some routines to dump the contents of entire BUFR files and the dump files can be compared. I remember when the BUFR library was updated to replace the Fortran routines for reading BUFR with C routines - there were number of non-zero (but very small) differences in reading BUFR files. You don't typically expect that with most of the releases - there is added functionality but the old stuff still works the same for the most part.

gmao-msienkie commented 5 months ago

So I am looking at some new code in the GSI to read high-density radiosondes. To do this it will require using the dynamic allocation version of the NCEP bufrlib. I see that the latest version of the NCEP BUFRLIB v12.0.1 has an extension added to support IODA. It makes me wonder if it would be useful to test our system with the latest BUFRLIB and possibly to have the BUFRLIB and possibly some of the other NCEP libraries mananged through modules instead of using old versions of the code built with the system. Of course this would require a lot of testing. Some libraries (like what we call "w3lib") also have a lot of local modifications and acquired some code that actually belonged with other NCEP software. I think our BUFRLIB is quite "vanilla" so most of our codes should work with an updated version.

edit: for my own testing it would be helpful to get a CMakefile for my own build for compiling for dynamic allocation with the standard Intel compiler.

mathomp4 commented 5 months ago

edit: for my own testing it would be helpful to get a CMakefile for my own build for compiling for dynamic allocation with the standard Intel compiler.

@gmao-msienkie Here you go:

https://github.com/GEOS-ESM/NCEP_Shared/pull/32

So you can checkout feature/mathomp4/dynamic-bufr locally and try it out. My guess is it'll work.