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

Ectrans4py tests rebased #199

Closed

Conversation

AlexandreMary
Copy link
Contributor

As requested by @wdeconinck,
here are some tests of ectrans4py feature.
They are python tests, as the purpose of this ectrans4py package is to call spectral transforms from Python (in EPyGrAM).

These tests were developed and passed on the original branch ACCORD-NWP/to_CY50 (PR #171) but I wasn't able to test after rebasing on ACCORD-NWP/to_CY50_rebased (PR #172) because of I wasn't able to link with FFTW at build time, which is now mandatory apparently.
Could you tell me (or rather document in the README) how to link properly with FFTW ?

Alexandre

AlexandreMary and others added 25 commits November 13, 2024 17:04
…ate (trans, etrans, biper); only single include directory
…ans/cpu/; (ii) create separate ectrans_etrans_* libraries instead of patching ectrans_* libraries; (iii) re-introduced FFT992, but put it under a switch WITH_FFT992 everywhere; compiling/running with FFT992 instead of FFTW is probably still broken; (iv) temporarily added ellips.F90, which in fact should go into fiat.
@FussyDuck
Copy link

FussyDuck commented Jan 23, 2025

CLA assistant check
All committers have signed the CLA.

@AlexandreMary
Copy link
Contributor Author

I added some info in the README about how to build the python wheel and run tests

@samhatfield
Copy link
Collaborator

Hey @AlexandreMary - thanks for this. This separate PR should make it easier to merge another PR that just contains CY50 contributions.

Regarding your FFTW question, I am guessing that your CMake configure step is failing with "cannot find FFTW package" or something?

Do you already have FFTW installed? You can compile it manually, or you can install it with a package manager.

Once it's installed, you should be able to make it known to CMake by setting the CMake variable FFTW_ROOT to the installation location:

https://ecbuild.readthedocs.io/en/latest/find/FindFFTW.html

If you have Intel MKL you can set the CMake variable FFTW_ENABLE_MKL to ON, and then MKL will be used. You'll then have to make MKL known to CMake by (I think) exporting MKLROOT to the MKL location.

Let me know how that goes. On every machine I've used it has "just worked" so I've not actually been through this myself. Happy to add some instructions to the README.md if you let me know if these instructions work.

@marsdeno
Copy link
Collaborator

The ectrans4py python interface relies on ctypesForPython, which appears to be licensed under the CECILL-C license.
Unfortunately, this is a problem for us.
Would it be possible to split the etrans work and the ectrans4py work into two separate PRs?
At that point we should be able to approve the etrans addition, needed for Arpege/IFS CY50 compatibility, quickly, and we can talk about how to handle ectrans4py after that?

@AlexandreMary
Copy link
Contributor Author

Hi Olivier,
if it is that problematic and can solve the issue, I can switch ctypesForFortran to Apache-2.0

As for splitting the first PR, OK, but now that the initial contribution has been rebased, and that Daan added some corrections, how can we get back to a version without ectrans4py ? With reverts ? :-(

Alexandre

@AlexandreMary
Copy link
Contributor Author

@samhatfield with FFTW_ROOT and FFTW_USE_STATIC_LIBS=ON I was able to compile and test with the present PR, cf. latest commit

@samhatfield
Copy link
Collaborator

Hi Olivier, if it is that problematic and can solve the issue, I can switch ctypesForFortran to Apache-2.0

As for splitting the first PR, OK, but now that the initial contribution has been rebased, and that Daan added some corrections, how can we get back to a version without ectrans4py ? With reverts ? :-(

Alexandre

Okay, here's what I propose: we break up this pull request into three smaller pull requests:

  1. Add etrans
  2. Add ectrans4py
  3. Other contributions

They can be merged in this order.

The 27 commits in this PR are neatly separated so we can build these three smaller PRs using cherry picking.

I've prepared a branch for 1. here.

It compares cleanly with develop: https://github.com/ecmwf-ifs/ectrans/compare/develop...samhatfield:add_etrans?expand=1

How does this look? If you're both happy with this, I can open the PR.

@AlexandreMary
Copy link
Contributor Author

OK for that,
can someone more competent than me can do the third branch ? Would be the 3 first commits of this branch (e2e73a4, 5ba37a2, ae51a5c) if I'm not mistaken. I was only the messenger of these changes, which mostly come from Ryad, and I tried to cherry-pick but there seems to be a few conflicts I don't feel confident solving myself.

I can do the second branch, atop of the first one from Sam.

@AlexandreMary
Copy link
Contributor Author

@ddegrauwe can you confirm you're OK with Sam's branch ?

@samhatfield
Copy link
Collaborator

OK for that, can someone more competent than me can do the third branch ? Would be the 3 first commits of this branch (e2e73a4, 5ba37a2, ae51a5c) if I'm not mistaken. I was only the messenger of these changes, which mostly come from Ryad, and I tried to cherry-pick but there seems to be a few conflicts I don't feel confident solving myself.

I can do the second branch, atop of the first one from Sam.

No problem, I'll handle the third branch/PR.

@samhatfield
Copy link
Collaborator

PR #203 opened.

@AlexandreMary
Copy link
Contributor Author

AlexandreMary commented Jan 28, 2025

I have an issue building upon your add_etrans branch:
which seems related to some of the changes in the 3 first commits of the initial branch ?
(maybe not so orthogonal then)

src/etrans/cpu/generated/ectrans_etrans_dp/internal/eledir_mod.F90:100:60:

  100 |     CALL EXEC_EFFTW(ITYPE,IRLEN,ICLEN,IOFF,KFC,TW%LALL_FFTW,PFFT)
      |                                                            1
Error: ‘lall_fftw’ at (1) is not a member of the ‘fftw_type’ structure

@samhatfield
Copy link
Collaborator

Good spot - it may be that we have to change the order of the PR creation so the "3. other contributions" comes first. Let me take a look :)

@samhatfield
Copy link
Collaborator

See PR #204. I am building PR #203 on top of #204.

@AlexandreMary
Copy link
Contributor Author

Replaced by #205

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.

7 participants