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

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

Open
mathomp4 opened this issue Nov 19, 2020 · 7 comments
Open

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

mathomp4 opened this issue Nov 19, 2020 · 7 comments
Assignees
Labels
bug Something isn't working question Further information is requested

Comments

@mathomp4
Copy link
Member

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: NOAA-EMC/NCEPLIBS-bufr#81 and another issue was linked to by @edwardhartnett about moving to dynamic by default: NOAA-EMC/NCEPLIBS-bufr#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)

@mathomp4 mathomp4 added bug Something isn't working question Further information is requested labels Nov 19, 2020
@gmao-msienkie
Copy link

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
Copy link
Member Author

@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
Copy link
Member Author

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
Copy link
Member Author

@gmao-msienkie
Copy link

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
Copy link

gmao-msienkie commented Jun 2, 2024

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
Copy link
Member Author

mathomp4 commented Jun 2, 2024

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:

#32

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants