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

Replace Numerial Recipes LU decomp routines with LAPACK ones #227

Merged

Conversation

AlexanderRichert-NOAA
Copy link
Contributor

@AlexanderRichert-NOAA AlexanderRichert-NOAA commented Jan 31, 2024

This PR removes code copied/pasted from Numerical Recipes (Fortran) and replaces it with standard LAPACK routines. Specifically, ludcmp and lubksb, which are routines for LU decomposition and subsequent linear equation solution, respectively, are replaced with {s,d}getrf and {s,d}getrs, respectively (s=single precision, d=double). For the CI, the Ubuntu workflows get these routines from OpenBLAS, the Intel one uses MKL routines, and the MacOS one uses the Apple-provided accelerate library. The BLAS/LAPACK provider can be specified to the CMake command with -DBLA_VENDOR=... where the available values can be found at https://cmake.org/cmake/help/latest/module/FindBLAS.html. In spack-stack, the BLAS/LAPACK provider will be OpenBLAS, and this setting will get propagated to downstream packages through ip-config.cmake (this resolves the issue I ran into with ufs-weather-model).

Tested on personal machine, Hera, and CI, including through updated Spack recipe.

Fixes #220

Test dependents:

  • ufs-weather-model (RTs pass; as far as I can tell the code paths in question don't get used)
  • ufs_utils (unit tests pass)
  • grib_util (unit tests pass)
  • UPP (need UPP team assistance)
  • gsibec (builds successfully)

@AlexanderRichert-NOAA AlexanderRichert-NOAA changed the title Replace Numerial Recipes LU-decomp routines with LAPACK ones Replace Numerial Recipes LU decomp routines with LAPACK ones Jan 31, 2024
@AlexanderRichert-NOAA AlexanderRichert-NOAA marked this pull request as ready for review January 31, 2024 23:13
@AlexanderRichert-NOAA
Copy link
Contributor Author

@GeorgeGayno-NOAA this is my solution for what we discussed about those two lapack_gen routines, let me know if you have any comments/concerns as far as the added dependency.

@edwardhartnett
Copy link
Contributor

Are we sure this will not change results somewhere?

@AlexanderRichert-NOAA
Copy link
Contributor Author

I tested with random numbers and the results are near identical, and both pairs of calls are covered in the unit testing. That said, I'm going to try to find some more "real life" examples to validate with.

@edwardhartnett
Copy link
Contributor

If this changes the UFS baseline test data, for example, that's something we need to clear with Jun...

@AlexanderRichert-NOAA
Copy link
Contributor Author

AlexanderRichert-NOAA commented Feb 1, 2024

These routines are not used directly by the forecast executable. As far as I know, these routines are used in:

  • ufs_utils
  • grib_util
  • UPP
  • gsibec

So I think that's where I want to look in terms of making sure I don't break anything...

Edit: Not sure about UFS WM, so I'm going to test it to be safe.

@edwardhartnett
Copy link
Contributor

Probably @GeorgeGayno-NOAA knows this is happening, right?

@AlexanderRichert-NOAA
Copy link
Contributor Author

Update: It appears these changes do affect UFS WM RT outputs, so definitely not merging this until everything is reconciled...

@AlexanderRichert-NOAA
Copy link
Contributor Author

Update/note to self: The issue with UFS WM is not these routines themselves (which don't appear to be used, at least in the RTs I've run), but having the LAPACK/OpenBLAS imported from ip creates numerical differences. There's probably a way to make this work, or, worst case scenario, copy and paste these two routines from Netlib LAPACK.

@AlexanderRichert-NOAA
Copy link
Contributor Author

@WenMeng-NOAA @HuiyaChuang-NOAA, can you please test these updates with UPP (long story short, splat now uses LU decomp routines from LAPACK/OpenBLAS rather than the Numerical Recipes routines currently in there)? I have a copy of the spack-stack-1.6.0 stack with these updates which can be used on Hera by pointing the modulefile to /scratch1/NCEPDEV/nems/Alexander.Richert/Ip5LAPACKtest/spack-stack-1.6.0/envs/ip_lapack_test/install/modulefiles/Core, and changing the versions for g2 and ip to 'develop' (plus get rid of sp from upp_common.lua+CMake config). Let me know if I can be of help in setting it up.

@AlexanderRichert-NOAA AlexanderRichert-NOAA merged commit ce1d621 into NOAA-EMC:develop Feb 29, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

calls to Numerical Recipes need to be removed?
2 participants