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

MétéoFrance contributions #204

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

Conversation

samhatfield
Copy link
Collaborator

This PR contains the "miscellaneous" contributions from PR #199.

@wdeconinck
Copy link
Collaborator

Looks OK!
I am just questioning if the new variable should not be referring to FFTW at all, so replacing LALL_FFTW with LALL_FFT?

@samhatfield
Copy link
Collaborator Author

Yes you're probably right. But then you should change also LD_ALL_FFTW to match, and that might break things at MF. @AlexandreMary do you think we can change the LD_ALL_FFTW argument of SETUP_TRANS to LD_ALL_FFT?

@AlexandreMary
Copy link
Contributor

That's indeed a change at the interface, that breaks compatibility.
We pass LD_ALL_FFTW=... in 4 routines of the IAL (aka ifs-source) repo.
So that's not big, but I would indeed prefer you do that renaming separately from this PR, so we could first use the next version as such with CY50 and CY50T1, before needing an update in our next common cycle CY51.

@wdeconinck
Copy link
Collaborator

wdeconinck commented Jan 29, 2025

@samhatfield I noticed that gpu/external/setup_trans.F90 does not match the changed interface include/ectrans/setup_trans.h with the new argument.

@samhatfield
Copy link
Collaborator Author

Good point. I will add that argument also to the GPU version and make it print a warning or something.

@samhatfield
Copy link
Collaborator Author

samhatfield commented Feb 5, 2025

Done.

Regarding the changes to GATH_SPEC, I note that after this PR, cpu/GATH_SPEC and gpu/GATH_SPEC are identical (modulo some whitespace). That must be because the changes proposed by MF in this PR were already merged into the CPU version some time ago. When I created this PR from cherry picking from MF's PR, I think those changes were applied a second time, but this time to the GPU source tree, because when we split into "cpu/gpu", we git mv'd the existing source tree to /gpu.

Long story short, I don't think this PR should be modifying anything under /gpu (except SETUP_TRANS). That's an error caused by the cherry pick, so I'm going to remove those changes.

@samhatfield
Copy link
Collaborator Author

Hi @wdeconinck and @AlexandreMary - as explained above, the suggested changes to GATH_SPEC (and GATH_SPEC_CONTROL) already seem to be in the cpu version. Hence the changes required to sync with MF are actually even smaller than originally thought. The only thing we need to add now is LD_ALL_FFTW. Could you take a look again and then we can merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants