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

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

Merged
merged 3 commits into from
Dec 8, 2023
Merged

Conversation

jbathegit
Copy link
Collaborator

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

jbathegit commented Dec 1, 2023

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

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

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 jack-woollen marked this pull request as ready for review December 7, 2023 18:38
@jack-woollen
Copy link
Contributor

@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
Copy link
Collaborator Author

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

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 :-)

@jack-woollen jack-woollen self-requested a review December 8, 2023 19:37
@jbathegit jbathegit merged commit ae4281a into develop Dec 8, 2023
6 checks passed
@jbathegit jbathegit deleted the jba_jwupdates branch December 8, 2023 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants