-
Notifications
You must be signed in to change notification settings - Fork 38
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
base: develop
Are you sure you want to change the base?
Add etrans #203
Conversation
62bd2e3
to
f768e41
Compare
a509fd2
to
476cf8d
Compare
…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.
4db6611
to
901f85c
Compare
bcf697c
to
cf12244
Compare
cf12244
to
2d58fa3
Compare
There was a problem hiding this 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!
Note: PR #204 should be merged before this one. |
Reminder that this (esp. commit 1bd0e01) requires |
Thank you for the reminder! |
There was a problem hiding this 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.
@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. 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. |
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. |
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. |
It was removed in commit 1bd0e01 - but maybe reintroduced in one of the rebasing steps ??? |
I cherry picked this commit which (erroneously?) added ellips to the etrans/cpu/internal directory. |
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. |
This PR adds the limited-area formulation of ecTrans, "etrans".
Actual work carried out by @ddegrauwe.