ACCORD-NWP CY50 contributions

[samhatfield]

This PR takes the branch from PR #171 and rebases it against develop for a clean merge.

This is a draft as further changes are required, e.g. adding an import for TW into FTINV etc.

[FussyDuck]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 5 committers have signed the CLA.

✅ AlexandreMary
✅ ddegrauwe
✅ samhatfield
❌ RyadElKhatibMF
❌ walidchikhi
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[samhatfield]

@ddegrauwe - I amended your commits which had ddegrauw@login05.leonardo.local as the "email address". Now you only need to sign the CLA once :)

[marsdeno]

I think something like
OpenMP::OpenMP_Fortran
is needed here, as omp_lib is used in the driver

[AlexandreMary]

I think something like OpenMP::OpenMP_Fortran is needed here, as omp_lib is used in the driver

I wonder if that wouldn't be the reason why we had to disable the PROGRAMS via an option when compiling in a Docker image for the build of the ectrans4py Python wheel ?

[samhatfield]

I think this check is redundant because it's done in the CMakeLists.txt one level up.

[wdeconinck]

Copyright statements are missing in every file I quickly browsed through.

[wdeconinck]

There are tests missing for etrans and ectrans4py.
This code is guaranteed to break at some point without any notice.
The benchmark code for etrans could be transformed into a test.

[wdeconinck]

This new logical variable LALL_FFTW is configurable via a new optional argument to SETUP_TRANS: LD_ALL_FFTW
This variable is then passed to EXEC_FFTW(..., LD_ALL)
The routine EXEC_FFTW itself however apparently does not make use of this variable, i.e. an unused argument. Unless I missed something, this variable is not used.

So I propose to revert all changes related to this variable in TPM_FFTW, and SETUP_TRANS.
Changing the API to external facing routines like SETUP_TRANS is something we should not do lightly.
It would involve also adding tests to exercise the new API, and one also needs to consider the GPU codepath, where FFTW is not used.

[samhatfield]

What about https://github.com/ecmwf-ifs/ectrans/blob/develop/src/trans/cpu/internal/tpm_fftw.F90#L276? It's used there.

[wdeconinck]

Apologies, I see now that LD_ALL is used in EXEC_FFTW after all.

[wdeconinck]

Why do we need an option to disable the programs?

If there is a good reason to do so, we should be consistent with other packages (e.g. eccodes) which use the feature "BUILD_TOOLS" for this.

[AlexandreMary]

See answer to comment by Olivier above

[wdeconinck]

Configuration with -DENABLE_ETRANS=ON seems broken. Possibly something went wrong with the rebase?

CMake Error at src/etrans/CMakeLists.txt:22 (target_include_directories):
  target_include_directories may only set INTERFACE properties on INTERFACE
  targets

I noticed that this PR patches a posteriori targets like trans_dp and trans_sp using target_sources. These targets are recently INTERFACE targets that wrap ectrans_dp and ectrans_sp which have precision suffixes to all modules and subroutines. The INTERFACE libraries trans_dp and trans_sp are there for backwards compatibility where the interface #include files for these targets are auto-generated and just redefine the symbols with suffix to be the old symbol without suffix.

The approach should be that new etrans_dp and etrans_sp targets should be created that link with trans_dp and trans_sp rather than patch them.

[wdeconinck]

Can the addition of ectrans4py be put on a backlog?

[AlexandreMary]

There are tests missing for etrans and ectrans4py. This code is guaranteed to break at some point without any notice. The benchmark code for etrans could be transformed into a test.

@ddegrauwe could you do the suggested transformation for etrans ?

[AlexandreMary]

Configuration with -DENABLE_ETRANS=ON seems broken. Possibly something went wrong with the rebase?

CMake Error at src/etrans/CMakeLists.txt:22 (target_include_directories):
  target_include_directories may only set INTERFACE properties on INTERFACE
  targets

I noticed that this PR patches a posteriori targets like trans_dp and trans_sp using target_sources. These targets are recently INTERFACE targets that wrap ectrans_dp and ectrans_sp which have precision suffixes to all modules and subroutines. The INTERFACE libraries trans_dp and trans_sp are there for backwards compatibility where the interface #include files for these targets are auto-generated and just redefine the symbols with suffix to be the old symbol without suffix.

The approach should be that new etrans_dp and etrans_sp targets should be created that link with trans_dp and trans_sp rather than patch them.

I think we didn't want to create separate libraries for etrans, and at the same time that etrans be as little intrusive to trans as possible. If you have something better to propose... (I have very little cmake experience).

[AlexandreMary]

Can the addition of ectrans4py be put on a backlog?

What do you mean ?

[wdeconinck]

• My proposal for etrans would be to have separate libraries, also making it clear that is is extending or adding functionality.
  @ddegrauwe if you want I can have a go at this, or discuss offline to assist you.

• What I meant with ectrans4py on backlog is that I'm not sure that this should be already added at this stage to ectrans.

  1. because it adds a lot of orthogonal things in one PR, so definitely I'd suggest a second PR just for this.
  2. because without having looked at it, I think ectrans is in such "flux" that it will be hard to maintain and test ectrans4py.

  If there's enough reason and urgency to add it at this point I can understand and support that.

[ddegrauwe]

I'll put the lam transforms in a separate library, following the single/double precision mechanism from the global transforms. Is it really necessary to have a "common" part (as in common to sp/dp)? What's the problem with just having sp/dp variants for all files?

I'll also add copyright statements (ECMWF+MeteoFrance; I'm not sure if ACCORD is a legal entity that can hold copyrights).

I'll also implement a test for the LAM transforms.

[wdeconinck]

Is it really necessary to have a "common" part (as in common to sp/dp)? What's the problem with just having sp/dp variants for all files?

The common part is not only common to sp/dp (cpu), but also to gpu_sp and gpu_dp.
The ultimate goal we're having is to have multiple backends (cpu/gpu+sp/dp) that can be chosen at runtime rather than at compile time.
Therefore we have this common library with common data structures like the domain decomposition and anything else that is precision independent, which can be shared and accessed from all backends.
Algortihms in the common library, like the domain decomposition have been made double-precision only. They should not give different result in single precision.

See this PR with latest changes in this regard: #141

[AlexandreMary]

Regarding ectrans4py:
it is quite important to MF/ACCORD, as it is used by our python package epygram that enable us to read and transform (to gridpoint) spectral fields from Arpege & LAM model files.
epygram is used quite broadly at MF and in ACCORD.
The python binding to the spectral transforms ectranspy has been around (under a different name) in ifsaux for a while (developed mainly by @SebastienRietteMTO), and used to be compiled by gmkpack. It has been "transfered" recently by the work of an ACCORD colleague, @walidchikhi, as ectrans4py in ectrans, to make it a standard python package, compilable with cmake, and published on PyPI.

These are simple wrappings to ectrans functions, and the python binding is done using ctypes under the hood. That said, I understand that upcoming changes in the interface routines of ectrans may require adaptations in these, and therefore tests are necessary.

Another option could be to make it a standalone project, out of ectrans though relying on it, but I think it would be a pity.

[wdeconinck]

OK @AlexandreMary fair enough, I won't object. I'd urge to have some tests for this soon. If nobody has time for this now that can come later but it would be great to have soon. That would also serve as use a minimal documentation.