search

NOAA-EMC / NCEPLIBS-bufr

The NCEPLIBS-bufr library contains routines and utilites for working with the WMO BUFR format.
Other
42 stars 19 forks source link

Jack Woollen's fix to optimize rdcmps.f routine #543

Closed jbathegit closed 7 months ago

jbathegit commented 8 months ago

Courtesy of @jack-woollen, and per the discussions in GSI/642, this patch to subroutine rdcmps optimizes performance for the majority of values which are encoded in fields of 32 bits or less.

jbathegit commented 8 months ago

Thanks again @jack-woollen for this fix, but before we can merge it we'll need to add a test case to the CI test suite, because right now the lines of code in the new else if (nbit<=64) branch are untested. So we'll basically need a compressed BUFR message which has at least one encoded value of more than 32 bits.

If you don't know of an example off the top of your head, then I can work to try to gin one up when I get a chance, but I may not get to this right away b/c I'll be busy with other stuff in the next few weeks up to the holiday break.

In fact, and come to think of it, we're going to need to update the emcrzdm tar archive of test cases anyway as part of an eventual solution for #533, so maybe we can combine both new test cases into the same archive update.

jack-woollen commented 8 months ago

Turns out 11.7.1 is the default on hera in hpc-stack:

/scratch2/NCEPDEV/nwprod/hpc-stack/libs/hpc-stack/modulefiles/compiler/intel/18.0.5.274 ------------------------------------------------------------------------------------------------------ bufr/11.4.0 bufr/11.5.0anaconda bufr/11.5.0 bufr/11.6.0 bufr/11.7.0 bufr/11.7.1 (D)

jbathegit commented 7 months ago

I've been working on generating a compressed BUFR message larger than 32 bits that we can add as a new test case in test_debufr.sh. However, while doing so, I uncovered a bug in subroutine wrcmps, as shown in this most recent commit.

jack-woollen commented 7 months ago

@jbathegit I don't know how I marked it ready for review as it says above. Let me know when it is and I will review it.

jbathegit commented 7 months ago

You must have accidentally clicked the button to change it from a draft pull request to a regular pull request, which by definition means it's ready for review.

However, it won't really be ready until I can push up the aforementioned test case to the emcrzdm archive and correspondingly modify test_debufr.sh. But before I modify the archive by pushing up the new test case, I also want to look at #533 as well.

Bottom line - no worries, and I'll let you know once this PR is really ready for review. Just FYI that I may not get all of the above done until after the holidays, b/c I'll be away at AGU next week.

jbathegit commented 7 months ago

OK, I had an unexpected block of time to work on this, so I've now added a test case to test_debufr.sh and renumbered the cases behind it within that same script.

@jack-woollen this PR is now ready for review, and I'd definitely appreciate if you have some time to now look this over and approve it, because I also have a fix for #533 ready to go right behind it :-)