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

can the _d version of g2 even work? #643

Open
edwardhartnett opened this issue Mar 13, 2024 · 10 comments
Open

can the _d version of g2 even work? #643

edwardhartnett opened this issue Mar 13, 2024 · 10 comments
Assignees
Labels
question Further information is requested

Comments

@edwardhartnett
Copy link
Contributor

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 edwardhartnett added the question Further information is requested label Mar 13, 2024
@edwardhartnett edwardhartnett self-assigned this Mar 13, 2024
@edwardhartnett
Copy link
Contributor Author

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

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

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

@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?

@edwardhartnett edwardhartnett moved this from To do to In progress in NCEPLIBS-g2-4.0.0 Jun 9, 2024
@AlexanderRichert-NOAA
Copy link
Contributor

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

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

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

Hang-Lei-NOAA commented Jun 10, 2024 via email

@edwardhartnett
Copy link
Contributor Author

edwardhartnett commented Jun 10, 2024

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

@Hang-Lei-NOAA
Copy link
Contributor

Hang-Lei-NOAA commented Jun 10, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
Status: In progress
Development

No branches or pull requests

4 participants