NOAA-EMC / NCEPLIBS-g2

Utilities for coding/decoding GRIB2 messages.
Other
7 stars 15 forks source link

can the _d version of g2 even work? #643

Open edwardhartnett opened 4 months ago

edwardhartnett commented 4 months ago

Is anyone using the _d version of g2?

There are several places where I don't understand how the g2 library can work with _d.

For example the g2 library is not very careful about the sizes of floats when dealing with IEEE values in the file, which are always 32-bit. It seems like the code is written so that the _d version will not work correctly.

For example, from addfield():

  ! Add Optional list of vertical coordinate values
  ! after the Product Definition Template, if necessary.
  if (numcoord .ne. 0) then
     do i = 1, numcoord
        coordlist_4(i) = real(coordlist(i), 4)
     end do
     call mkieee(coordlist_4, coordieee, numcoord)
     call g2_sbytescr(cgrib, coordieee, iofst, 32, 0, numcoord)
     iofst = iofst + (32 * numcoord)
  endif

Yet mkieee() expects 4-bytes for it's first two parameters:

 subroutine mkieee(a, rieee, num)
    implicit none

    real(4), intent(in) :: a(num)
    real(4), intent(out) :: rieee(num)
    integer, intent(in) :: num

Since coordieee is a double in the _d version of the library (and needs to be so that it can work in the g2_sbytescr() call immediately following.

OK, so this could never have worked.

Are we producing and testing the _d version of the library pointlessly? Can we just release the _4 version? That would be a lot better - half the build time, half the testing, and half the install. Also the code could be a lot simpler.

@AlexanderRichert-NOAA @GeorgeVandenberghe-NOAA @GeorgeGayno-NOAA do you know of anyone using the _d version of the g2 library?

edwardhartnett commented 4 months ago

I am going to turn off the _d build in the cmake by default.

It works and I will continue to test it, but let's do a release with it turned off and see if that's a problem for anyone...

AlexanderRichert-NOAA commented 4 months ago

Sorry for the slow reply-- g2::g2_d appears to be used in UFS_UTILS: sorc/emcsfc_snow2mdl.fd/CMakeLists.txt and sorc/chgres_cube.fd/CMakeLists.txt; as well as grib_util: src/copygb2/CMakeLists.txt.

edwardhartnett commented 4 months ago

OK, weird. I wonder if it only works read-only?

In any case, I will investigate further in the next release. For now, we will continue as we have, with a _4 and a _d build.

edwardhartnett commented 1 month ago

@AlexanderRichert-NOAA and @Hang-Lei-NOAA do we build with _d in spack-stack or anywhere else at NOAA?

If not, then I propose to eliminate the _d build, after the next release.

What do you think?

AlexanderRichert-NOAA commented 1 month ago

We build both versions in spack-stack. UFS UTILS still uses _d. We could see what would be involved in changing it over to 4; I'm definitely in favor of reducing the extra "" builds whenever and wherever possible.

edwardhartnett commented 1 month ago

Does anyone use the _8 build of UFS_UTILS? @GeorgeGayno-NOAA do you know?

As mentioned at the top of the issue, I believe there would be some subtle bugs in the _8 version of the library, not currently explored in testing. (And I guess that's my next step.)

But if no one ever used the _8 version of g2 (or UFS_UTILS), it would be great to drop them. They add a lot of extra work - is there a reason for us to do all that work, or is this a "feature" that no one uses?

GeorgeGayno-NOAA commented 1 month ago

Does anyone use the _8 build of UFS_UTILS? @GeorgeGayno-NOAA do you know?

As mentioned at the top of the issue, I believe there would be some subtle bugs in the _8 version of the library, not currently explored in testing. (And I guess that's my next step.)

But if no one ever used the _8 version of g2 (or UFS_UTILS), it would be great to drop them. They add a lot of extra work - is there a reason for us to do all that work, or is this a "feature" that no one uses?

UFS_UTILS only uses the "_4" and "_d" versions. And I would suspect most users do not use the "_8" version.

Hang-Lei-NOAA commented 1 month ago

It is true that _8 may not be necessary. When Kyle was here, we used to try to use only _4, but many users complain about no _d.

On Mon, Jun 10, 2024 at 8:50 AM GeorgeGayno-NOAA @.***> wrote:

Does anyone use the _8 build of UFS_UTILS? @GeorgeGayno-NOAA https://github.com/GeorgeGayno-NOAA do you know?

As mentioned at the top of the issue, I believe there would be some subtle bugs in the _8 version of the library, not currently explored in testing. (And I guess that's my next step.)

But if no one ever used the _8 version of g2 (or UFS_UTILS), it would be great to drop them. They add a lot of extra work - is there a reason for us to do all that work, or is this a "feature" that no one uses?

UFS_UTILS only uses the "_4" and "_d" versions. And I would suspect most users do not use the "_8" version.

— Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/NCEPLIBS-g2/issues/643#issuecomment-2158259537, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKWSMFEWYYEV27QTA45SM33ZGWOIVAVCNFSM6AAAAABEUHOF3CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJYGI2TSNJTG4 . You are receiving this because you were mentioned.Message ID: @.***>

edwardhartnett commented 1 month ago

@Hang-Lei-NOAA do you know specifically who is using the _d?

Hang-Lei-NOAA commented 1 month ago

@Edward Hartnett - NOAA Affiliate @.***> After an investigation, I believe that it is safe to remove _8.

The only code that using _8 build which I can find, are: https://github.com/NOAA-EMC/gfs-utils/blob/develop/src/fbwndgfs.fd/CMakeLists.txt https://github.com/NOAA-EMC/WAFS/blob/develop/sorc/wafs_makewafs.fd/CMakeLists.txt

However, we have implemented the only _4 for bacio for such a long time, there is no trouble for them.

On Mon, Jun 10, 2024 at 10:30 AM Edward Hartnett @.***> wrote:

@Hang-Lei-NOAA https://github.com/Hang-Lei-NOAA do you know specifically who is using the _8?

— Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/NCEPLIBS-g2/issues/643#issuecomment-2158528122, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKWSMFHGWTJI5U3DPMRIGODZGW2BNAVCNFSM6AAAAABEUHOF3CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJYGUZDQMJSGI . You are receiving this because you were mentioned.Message ID: @.***>