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.

Sign up for GitHub

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

Fix CMakeLists.txt of NCEP_bufr for gnu compiler #26

Merged
merged 2 commits into from
Nov 17, 2022
Merged

Fix CMakeLists.txt of NCEP_bufr for gnu compiler #26

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c18b368
Fix CMakeLists.txt of NCEP_bufr for gnu compiler
gmao-cda Nov 14, 2022
213016b
Merge branch 'main' into bugfix/cda/bufr_v1_2_0_cmakelists_gnu
gmao-cda Nov 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion NCEP_bufr/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,11 @@ if (INTEGER_KIND STREQUAL "i8")
endif()


target_compile_definitions (${this} PRIVATE $<$<COMPILE_LANGUAGE:C>:STATIC_ALLOCATION>)
if (CMAKE_C_COMPILER_ID MATCHES Intel)
target_compile_definitions (${this} PRIVATE $<$<COMPILE_LANGUAGE:C>:STATIC_ALLOCATION>)
elseif (CMAKE_C_COMPILER_ID MATCHES "GNU")
target_compile_definitions (${this} PRIVATE $<$<COMPILE_LANGUAGE:C>:DYNAMIC_ALLOCATION>)
endif ()
Comment on lines +378 to +382
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmao-cda Why do we not want both compilers to do the same thing?

The differences you reported elsewhere suggest that there was a change in the library that requires an update to DYNAMIC, but then it should be for all compilers. E..g, the logic above is problematic for say the NAG compiler. (Granted we only use NAG for MAPL at the moment, but ... we have ambitions.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tclune The main issue is that Intel at the moment uses STATIC_ALLOCATION. I did ask about this in #17 but I am wary of touching code that is only really tested in the ADAS. I think we should move to DYNAMIC_ALLOCATION for all compilers, but I don't know how to test this stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have Intel compilers installed on my old macbook air. When I return home after work, I can test if switching to "DYNAMIC_ALLOCATION" for Intel will generate workable bufr libs when decoding my benchmark observations stored in BUFR format.

At least we can get some clues from this test.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we cannot test with ADAS, then maybe we should instead introduce a CMAKE "option", with the default being STATIC. One could then specify DYNAMIC on the command line.

The utility of this suggestion depends on how many non-DAS people need this library, and on whether we can come up with a good name for the option.

Just thinking out loud.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mathomp4 To have more thorough tests on BUFRLIB, essentially those subs read_*.f90 under GSI will decode bufr and prepbufr files and write out their decoded datasets in binary. We can compare if those decoded binary datasets have same results using the original and modified bufrlibs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recompiled bufr libs with intel compilers (comp/intel/2021.2.0) on Discover,
with DYNAMIC_ALLOCATION applied tor both ifort and icc.

Results:

There is no obvious error using DYNAMIC_ALLOCATION for Intel compiler (comp/intel/2021.2.0):

  1. combfr.x can still link correctly to NCEP_bufr_r4i4,
  2. One experiment decoding my own PrepBUFR data shows the bufr data is correctly decoded.
cda@discover34:bufr_utils>./validate_build.sh
================================================================================
                  SYNOPSIS OF PREPBUFR DECODER using BUFRLIB
                              by TAKEMASA MIYOSHI
--------------------------------------------------------------------------------
 TOTAL NUMBER OF READ-IN RECORDS:    370686
 TOTAL NUMBER OF WRITTEN RECORDS:    301945 (levels/variables separately)
--------------------------------------------------------------------------------
  ADPUPA  AIRCAR  AIRCFT  SATWND  PROFLR  VADWND  SATEMP  ADPSFC  SFCSHP  SFCBOG
    1370       0    1468  117085     327    1580       0  108483   12804       0
   99576       0    1514   95648   12312   13432       0   49393   18264       0
--------------------------------------------------------------------------------
  SPSSMI  SYNDAT  ERS1DA  GOESND  QKSWND  MSONET  GPSIPW  RASSDA  WDSATR  ASCATW
       0       0       0       0       0       0     838     155    5903  120673
       0       0       0       0       0       0       0       0   11806       0
--------------------------------------------------------------------------------
  OTHERS
       0
       0
================================================================================
-------------------------------------------------------
check difference for time step: t-3
 fnin, nobs=t-3.dat       19050
 fnin2, nobs2=t-3.dat_test       19050
 rdiff=  9.9999998E-03
 total error #=           0
-------------------------------------------------------
check difference for time step: t-2
 fnin, nobs=t-2.dat       20077
 fnin2, nobs2=t-2.dat_test       20077
 rdiff=  9.9999998E-03
 total error #=           0
-------------------------------------------------------
check difference for time step: t-1
 fnin, nobs=t-1.dat      102671
 fnin2, nobs2=t-1.dat_test      102671
 rdiff=  9.9999998E-03
 total error #=           0
-------------------------------------------------------
check difference for time step: t
 fnin, nobs=t.dat      116746
 fnin2, nobs2=t.dat_test      116746
 rdiff=  9.9999998E-03
 total error #=           0
-------------------------------------------------------
check difference for time step: t+1
 fnin, nobs=t+1.dat       18250
 fnin2, nobs2=t+1.dat_test       18250
 rdiff=  9.9999998E-03
 total error #=           0
-------------------------------------------------------
check difference for time step: t+2
 fnin, nobs=t+2.dat       18953
 fnin2, nobs2=t+2.dat_test       18953
 rdiff=  9.9999998E-03
 total error #=           0
-------------------------------------------------------
check difference for time step: t+3
 fnin, nobs=t+3.dat        6198
 fnin2, nobs2=t+3.dat_test        6198
 rdiff=  9.9999998E-03
 total error #=           0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to ping @gmao-msienkie and see if she thinks this looks good. If so, we might do a two-step process here. One, take in the code in this PR where it's still static on Intel and make a release. Then maybe make another release where we go full DYNAMIC.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was checking on this issue. It looks like there are a number of the "read_*" routines where the code has "call closbf" prior to any "call openbf" - which is what had caused a crash before with the version of BUFRLIB we are currently using when compiled with dynamic allocation. I had started a branch to fix this problem in 'read_iasi.f90' but killed my branch when I realized the extent of this "feature" in our code.
You noted in #17 that the newer version of the BUFRLIB would just print a warning message instead of exiting. That could work but I don't think that Ricardo would care to have numerous warning messages (one for each processor executing a problematic "read" routine) in the analysis log file. It looks like that we are still on 11.3.0 (see bvers.f), are there any plans to update the library.
I happened to get a question today regarding a newer version of 'read_cris.f90' someone copied from NCEP. When comparing with our current code I noticed the "call closbf" prior to the "call openbf" had been removed. Perhaps there has been some code cleaning as well as updates for data changes and science changes.
I think that NCEP is currently using a newer version of the BUFR library (11.4.0) in their operations. It looks like from the release notes that a number of features have been added to the library in the latest versions, as well as switching to compiling only with dynamic allocation. I am kind of thinking that when we bring in newer code for the GSI from NCEP we will want to also update the BUFR library at least to the version corresponding to what they are using with their code. At that point the people responsible for components that use the BUFRlib can carry out zero-diff tests to satisfy themselves that the code is working and not changing results.

target_compile_definitions (${this} PRIVATE $<$<COMPILE_LANGUAGE:C>:MAXNC=600>)
target_compile_definitions (${this} PRIVATE $<$<COMPILE_LANGUAGE:C>:MXNAF=3>)
target_compile_definitions (${this} PRIVATE $<$<COMPILE_LANGUAGE:C>:UNDERSCORE>)
Expand Down