JGribX icon indicating copy to clipboard operation
JGribX copied to clipboard

GRIB2 IEEE packing

Open Martin-Imrisek opened this issue 10 months ago • 7 comments

Dear spidru,

I really appreciate the work you have done on JGribX. I have developed the Grib2RecordGDS class for Lambert Local Area Modelling (though it's not yet fully complete) and new unpacking method for double (because for geopotential we are using the grib_ieee packing) precision data.

However, the data type is in conflict with Grib2RecordDS class, because the class Grib2RecordDS stores data as floats.

I would like to ask you how you suggest to proceed? Create new Grib2RecordDS class for double data or something else? Maybe interface can do the trick?

Thank you for your reply in advance, Martin

Martin-Imrisek avatar Feb 18 '25 11:02 Martin-Imrisek

Hi there,

Thank you for kind words, glad you found JGribX useful :)

That sounds great! Yes, I can look into adding support for type double in the Data Section. Could you create a pull request with what you have (even if it doesn't build) so that I can have a look at what you currently have?

spidru avatar Feb 23 '25 09:02 spidru

Hello, Thank you for your reply. The pull request was created. Martin

Martin-Imrisek avatar Feb 24 '25 09:02 Martin-Imrisek

Had a quick look but didn't find time these last few days. Should find some time this week though, thanks for the PR!

spidru avatar Mar 02 '25 19:03 spidru

Hi @MartinImrisekSHMU,

I finally had some time to have a proper look into the DS implementation. So the issue is that all the other packing types use float32, which is why I've chosen float as the DS data type. I could simply change this to double, but this would essentially double the RAM usage size for the other packing types unnecessarily. In addition, this IEEE floating point packing type also supports 128-bit floating point values, which of course would still suffer from precision loss with double.

The ideal solution I see here is that I first rework the DS implementation to a "lazy loading" approach, where the values are read out from the file only as needed. This is in contrast with the current implementation, which loads the entire DS into memory. Then, adding support for 64-bit or even 128-bit data values would not have a significant impact on memory usage. Of course, this would probably take quite some time for me to do.

Eager to hear your thoughts on this.

spidru avatar Mar 15 '25 18:03 spidru

The simplest short-term "solution" I see in the meantime is that we essentially merge your changes as-is, with the warning in place regarding the precision loss.

Perhaps a question more specific for your current use case: Would the precision loss from float64 to float32 have a significant impact on your calculations? Keeping in mind that, generally speaking, information is only lost after 7 significant figures.

spidru avatar Mar 16 '25 12:03 spidru

Dear @spidru,

sorry for missing your reply two weeks ago and thank you for your time. I can confirm that the precision loss has no effect in my case (surface geopotential). However, I did switch to double precision of data, which indeed doubled the RAM usage, maybe I'll revert it to the float32.

Since my PR I have developed also the Lambert projection, reworked the reading of records to be enable writing the GRIB messages to HDD for further usage and developed the simple packing according to ECCODES. Because we would like to use it for compute total precipitation, surface friction etc. Currently I am exploring the CCSDS packing (based on https://github.com/MathisRosenhauer/libaec.git). However, it will be sidetracked for a while and and the extraction of subdomains from data and updating GRIB messages accordingly will have more priority.

Regards, Martin

Martin-Imrisek avatar Mar 26 '25 10:03 Martin-Imrisek

Hi @MartinImrisekSHMU,

No worries for the delayed responses, I'm also quite slow at this apparently :)

Thanks for confirming that the precision loss has negligible effect on your use case. With this in mind, I propose to merge in your PR #24 as-is. I'll wait until next weekend in case you'd like to refine the PR in light of this decision, then I'll merge it in.

Of course, any other features/improvements are welcome, just let me know via new issue or PR :) Thanks again for your contribution!

spidru avatar Apr 21 '25 16:04 spidru