Contributions from JCSDA

benjaminmenetrier commented 4 years ago

Hello, this PR contains recent contributions added at JCSDA (@mmiesch, @markjolah, @rhoneyager, @cmgas, @ytremolet and @benjaminmenetrier) to the develop branch of the ECMWF repo:

Add configuration functions (get_size, set_array_string and get_array_logical):
- src/fckit/module/fckit_configuration.F90
- src/fckit/module/fckit_configuration.cc
- src/fckit/module/fckit_configuration.inc
Add interfaces to eckit geometry and kdtree, with tests:
- src/fckit/CMakeLists.txt
- src/fckit/module/fckit_geometry.F90
- src/fckit/module/fckit_geometry.cc
- src/fckit/module/fckit_kdtree.F90
- src/fckit/module/fckit_kdtree.cc
- src/tests/CMakeLists.txt
- src/tests/test_geometry.F90
- src/tests/test_kdtree.F90
Add directory label for CDash:
- CMakelists.txt
Add a constructor for fckit_mpi_comm:
- src/fckit/module/fckit_mpi.fypp
Remove developer-only warnings from release builds:
- cmake/add_fctest.cmake

I don't know why GitHub says "128 files changed" and does not show the develop branch of ECMWF repo on the left side of the comparison, as it should.

FussyDuck commented 4 years ago

All committers have signed the CLA.

wdeconinck commented 4 years ago

Hi Benjamin, could you please split this in several PRs (configuration, kdtree, mpi, ... ) that are rebased off the most recent fckit develop branch, and resolve any conflicts you may have? It now looks like you touched 128 files, and we cannot review.

benjaminmenetrier commented 4 years ago

Hello @wdeconinck. I think I need some help. Either there is an issue with GitHub, or there is something I don't understand.

To compare the branch jcsda:develop-jcsda with respect to the branch ecmwf:develop on my machine, I go into the directory of the JCSDA fork of fckit, checkout to the develop-jcsda branch, add the ECMWF remote and diff:

$ cd fckit
$ git checkout develop-jcsda
$ git remote add -f ecmwf https://github.com/ecmwf/fckit
$ git remote update
$ git diff --stat remotes/ecmwf/develop develop-jcsda
 CMakeLists.txt                           |   1 +
 cmake/add_fctest.cmake                   |   4 ++
 src/fckit/CMakeLists.txt                 |   6 +-
 src/fckit/module/fckit_configuration.F90 |  85 +++++++++++++++++++++++++-
 src/fckit/module/fckit_configuration.cc  |  21 ++++++-
 src/fckit/module/fckit_configuration.inc |  26 ++++++++
 src/fckit/module/fckit_geometry.F90      |  85 ++++++++++++++++++++++++++
 src/fckit/module/fckit_geometry.cc       |  46 ++++++++++++++
 src/fckit/module/fckit_kdtree.F90        | 100 +++++++++++++++++++++++++++++++
 src/fckit/module/fckit_kdtree.cc         |  89 +++++++++++++++++++++++++++
 src/fckit/module/fckit_mpi.fypp          |   8 +++
 src/fckit/module/fckit_shared_object.F90 |   2 +-
 src/tests/CMakeLists.txt                 |  16 ++++-
 src/tests/test_geometry.F90              |  87 +++++++++++++++++++++++++++
 src/tests/test_kdtree.F90                |  62 +++++++++++++++++++
 15 files changed, 631 insertions(+), 7 deletions(-)

There are 9 files modified, and 6 new files, as expected.

On GitHub, I am requesting a merge of jcsda:develop-jcsda into ecmwf:develop. Surprisingly, GitHub finds 128 files changed, and it does not display the ecmwf:develop branch on the left side of the comparison tab. For instance, if you look at the line 24 of CMakeLists.txt, on the left side you get:

ecbuild_requires_macro_version( 2.7 )

whereas if you look at the same file in the develop branch of the ECMWF repo, you see:

set(CMAKE_CXX_STANDARD 11)

This is very misleading! Is there something I don't understand?

benjaminmenetrier commented 4 years ago

Good news, @mmiesch had the solution: I merged ecmwf:develop into jcsda:develop-jcsda with the "ours" strategy:

git checkout develop-jcsda
git merge -s ours ecmwf/develop

Now, only the 15 changed files appear. @wdeconinck, should I split this PR to make the review easier or is it OK?

wdeconinck commented 4 years ago

I agree with most changes in the PR except the kd-tree and geometry.

eckit's KDTree has refrained to make any assumptions on what is a Point and Payload, radius of Earth, etc... and I don't think fckit should either.

It so happens that at the moment I am creating a more custom KDTree in Atlas based on eckit's where such assumptions are better placed. It would be better if we could instead move such functionality to Atlas instead. I expect that the C++ version of the KDTree could be available before the end of this week.

benjaminmenetrier commented 4 years ago

Hi @wdeconinck. I understand your concerns about genericity.

For the geometry, the new routines in fckit are just interfaces to existing ones in eckit without any further assumptions or calculations. To stay closer to eckit names, maybe we should change:
- sphere_distance into unit_sphere_distance
- sphere_lonlat2xyz into unit_sphere_convert_spherical_to_cartesian
- sphere_xyz2lonlat into unit_sphere_convert_cartesian_to_spherical
For the kdtree, we would be glad to move to your new version using ATLAS. However, we would need some Fortran interfaces. Maybe I can help writing them?

wdeconinck commented 4 years ago

I worked on merging all changes except the geometry and kdtree into latest develop branch. You can try to reintegrate it again with this branch to see if all is still OK.

Regarding the sphere functions, I would still prefer not to have "some" geometry related stuff in fckit and others in atlas, so as to keep everything more tidy in the same place.

Thanks for your offer @benjaminmenetrier; as soon as the atlas KDTree is ready I will create an issue and assign you to create the Fortran interfaces.

benjaminmenetrier commented 4 years ago

Thanks @wdeconinck, I just merged your new develop into jcsda:develop-jcsda and tested it. Everything looks good. I will try to use ATLAS for the geometry aspects instead of FCKIT. What do you think about making the fckit_shared_object_c_ptr method public in src/fckit/module/fckit_shared_object.F90?

wdeconinck commented 4 years ago

What do you think about making the fckit_shared_object_c_ptr method public in src/fckit/module/fckit_shared_object.F90?

It had to be commented for compatibility with a PGI compiler. I had reported the bug to the NVIDIA/PGI. I have not checked if it is already fixed in latest compiler though.

benjaminmenetrier commented 4 years ago

It is used in one of our repos (UFO), but I'll see if we can get rid of it.

wdeconinck commented 4 years ago

Hold on @benjaminmenetrier we can try to enable it again, perhaps by detecting if PGI compiler is used. At least if you're not using PGI compiler it will work then.

The workaround that I currently use is to replace:

<object>%c_ptr()

with

<object>%shared_object_%cpp_object_ptr

I had created macros for this, e.g. here: https://github.com/ecmwf/fckit/blob/0.7.0/src/fckit/fckit.h.in#L46

benjaminmenetrier commented 4 years ago

OK, thanks. We don't use PGI at JCSDA, so the workaround is fine for us.

wdeconinck commented 4 years ago

@benjaminmenetrier Current develop branch also re-enabled the c_ptr() member function. There should also be no need for the workaround if you are using PGI compiler 19.4 or higher, or less than 17.10

ecmwf / fckit

Contributions from JCSDA #11