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

Add etrans #203

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open

Add etrans #203

wants to merge 15 commits into from

Conversation

samhatfield
Copy link
Collaborator

This PR adds the limited-area formulation of ecTrans, "etrans".

Actual work carried out by @ddegrauwe.

Copy link
Collaborator

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks perfect now. Thanks @ddegrauwe for addressing my suggestions!

@samhatfield
Copy link
Collaborator Author

Note: PR #204 should be merged before this one.

@AlexandreMary
Copy link
Contributor

Reminder that this (esp. commit 1bd0e01) requires fiat PR ecmwf-ifs/fiat#30

@wdeconinck
Copy link
Collaborator

Reminder that this (esp. commit 1bd0e01) requires fiat PR ecmwf-ifs/fiat#30

Thank you for the reminder!

Copy link
Collaborator

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for overlooking this earlier: I don't agree with the move of ellips routines to fiat. I suggest to add them to the ectrans_common library instead, within ectrans repository. That also removes the dependency of merging ecmwf-ifs/fiat#30.

@AlexandreMary
Copy link
Contributor

@wdeconinck the move of ellips was done for the needs of the externalisation of our FA format library (taken out of ifsaux), in the spirit of what has been done for ectrans. Indeed, FA calls ellips in order to determine the size of arrays for spectral fields, when reading spectral fields.
If we move it back to ectrans, it introduces a dependency of FA to ectrans, which is a bit far-fetched, especially as FA can be used in context that never deal with spectral fields (as if eccodes depended on ectrans for spherical harmonics fields).

Is that a design issue, I don't know, but one of the consequences of the desintrications of arpifs/trans/ifsaux. But maybe we can make that dependency optional there.
@ddegrauwe, @walidchikhi, any opinion on this ?

@wdeconinck
Copy link
Collaborator

wdeconinck commented Jan 29, 2025

Actually it seems ellips.F90 is still part of ectrans in this PR: src/etrans/cpu/internal/ellips.F90 so that this PR already should not depend on ecmwf-ifs/fiat#30 (?)

I suggest to add another copy of the ellips routine in the FA library. The routine is small and trivial.
Also better to change the symbol name within etrans as it is a private implementation detail, and avoids symbol clash with the other one.

@wdeconinck
Copy link
Collaborator

I agree that FA should not depend on ectrans, just as eccodes does not. eccodes however does reimplement some functions needed to support spectral fields, and does so in the C-language.

@AlexandreMary
Copy link
Contributor

Actually it seems ellips.F90 is still part of ectrans in this PR: src/etrans/cpu/internal/ellips.F90 so that this PR already should not depend on ecmwf-ifs/fiat#30 (?)

I suggest to add another copy of the ellips routine in the FA library. The routine is small and trivial. Also better to change the symbol name within etrans as it is a private implementation detail, and avoids symbol clash with the other one.

It was removed in commit 1bd0e01 - but maybe reintroduced in one of the rebasing steps ???

@samhatfield
Copy link
Collaborator Author

samhatfield commented Jan 29, 2025

I cherry picked this commit which (erroneously?) added ellips to the etrans/cpu/internal directory.

@ddegrauwe
Copy link
Collaborator

Yes, I reintroduced ellips.F90 in ectrans to be able to build without modifying the fiat version. Sorry about the confusion this caused.

Cleanest solution to me seems indeed to duplicate ellips in ectrans and in fa.

ddegrauwe and others added 12 commits February 6, 2025 08:54
…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.
Commit 8622da1 changed D%NSTAGTF from
JPIM to JPIB.
@lukasm91
Copy link
Collaborator

lukasm91 commented Feb 6, 2025

Note there are various mentions of FFT992 added in this commit. Can we drop those ifdefs? In general it would probably make sense to check the diffs of the files that now exist twice (like for example ftinvad_mod.F90 is likely a copy of ftinvad_mod.F90, with maybe a few, but only very few modifications)

@samhatfield
Copy link
Collaborator Author

Note there are various mentions of FFT992 added in this commit. Can we drop those ifdefs? In general it would probably make sense to check the diffs of the files that now exist twice (like for example ftinvad_mod.F90 is likely a copy of ftinvad_mod.F90, with maybe a few, but only very few modifications)

You mean eftinvad_mod.F90 looks like ftinvad_mod.F90?

I think the ff992 of trans/algor/fft992.F90 was previously used by etrans. Now the former has been deleted and we rely on FFTW only for ecTrans. I guess @ddegrauwe left in the #ifdef WITH_FFT992 for a reason, maybe to add fft992.F90 to etrans in future?

If there is no plan to use fft992 in etrans going forward, then yes I suggest we remove all instances of #ifdef WITH_FFT992.

@ddegrauwe
Copy link
Collaborator

Exactly. It hasn't been decided/agreed in our community whether we can get rid of FFT992. That's why I kept these calls under an #ifdef. If this is annoying, I guess it can be removed alltogether: putting it back isn't such a big issue.

About the duplication of routines: most routines for the global case have a LAM counterpart (e.g. ftinvad_mod vs eftinvad_mod). While the functionality is the same, some details may be different. Where possible, duplication is avoided and global routines are called directly from the LAM code (e.g. ftinv is called from eftinv_ctl).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved-for-ci enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants