GEOS, FMS, Constants...and CMake

[mathomp4]

TL;DR: GEOS uses different constants than FMS. What is the best way to handle this, especially in CMake FMS?

--

Recently, @danholdaway informed me that GEOS might need to catch up with Modern FMS. We haven't updated in a while mainly because "Don't break things that are working™" so we are sort of close to 2019.01.02.

If you look at our current constants.F90 on our fork, the main difference is that we use our GEOS/MAPL values for the constants so the FMS Earth looks like MAPL Earth and we control that via an #ifdef much like how the GFS constants are triggered:

option(GFS_PHYS              "Enable compiler definition -DGFS_PHYS"              OFF)

Now a while back, I asked about this topic in #155 but back then FMS wasn't as fully in CMake-land, but now it is. So it got me to thinking: Could we now figure out a way to handle MAPL constants in FMS in a way that is palatable to GFDL? I suppose I could ask very nicely if we could add the MAPL constants as well and add one more definition. But three sets of constants in one file is near the limit of annoying! Not impossible, but you might balk at this.

So another thought I had was maybe add an extra constants file? That is, not just constants/constants.F90 but say constants/extern/MAPL_constants.F90 or something and then something like this in CMakeLists.txt:

if (MAPL_PHYS)
  list(APPEND fms_fortran_src_files constants/extern/MAPL_constants.F90)
else ()
  list(APPEND fms_fortran_src_files constants/constants.F90)
endif ()

That way, we supply a constants_mod just a different one?

I suppose in the end my hope is that GEOS could get to the point where FMS is "just another library" rather than needing a fork which might have a single file different. (I know @tclune has thoughts on better ways to do this, i.e., supply constants as a dictionary, but that's a much longer-term project, if it's even possible.)

Note: I am currently trying to build the current FMS as "just a library" via CMake and see if I can get GEOS to use it. Obviously it'll have the wrong constants, but if I can do that, then that brings GEOS' use of FMS a lot closer!

[bensonr]

@mathomp4 - I think this is something we can discuss to come up with the best solution for all.

[mathomp4]

@bensonr (and I guess @aerorahul as it sort of involves #806) - After working with @tclune, I think I can now use 2021.03 with GEOS constants with the following updates.

1. I have to patch up constants/constants.F90 to have the MAPL/GEOS constants
2. I have to update CMakeLists.txt to have:

option(MAPL_MODE             "Enable compiler definition -DMAPL_MODE"             OFF)
if(MAPL_MODE)
  set(CMAKE_POSITION_INDEPENDENT_CODE ON)
  list(APPEND fms_defs MAPL_MODE)
endif()

I'm still doing testing, but I think this is what is needed.

(Note: We aren't wed to MAPL_MODE in anyway, just an old naming convention. Probably GEOS_PHYS would match GFS_PHYS a lot more.)

[bensonr]

@mathomp4 - I think we need to be descriptive and I'd suggest the macro be NASA_GEOS. While the GFS_PHYS was chosen early purely to satisfy a need, I think it should be changed to something like UFS_GFS or UFS_SYSTEM. To make things easier to manage, I could create a separate .h for each modeling system and include the appropriate set based on the macro setting. Your thoughts?

[mathomp4]

@bensonr - Any way you'd want to support it--different .h or .inc or .F90 files--would be fine by me! I can definitely work with you to make sure things are in a good Doxygen-y way and have the right MAPL/GEOS numbers.

[mathomp4]

@bensonr (and I guess @aerorahul as it sort of involves #806) - After working with @tclune, I think I can now use 2021.03 with GEOS constants with the following updates.

1. I have to patch up constants/constants.F90 to have the MAPL/GEOS constants
2. I have to update CMakeLists.txt to have:

option(MAPL_MODE             "Enable compiler definition -DMAPL_MODE"             OFF)
if(MAPL_MODE)
  set(CMAKE_POSITION_INDEPENDENT_CODE ON)
  list(APPEND fms_defs MAPL_MODE)
endif()

I'm still doing testing, but I think this is what is needed.

Well, I did some testing and, miracle of miracles, in-Baselibs CMake FMS 2021.03 built with these edits and your compile options seems to be zero-diff to our current FMS inside GEOS with our compile options. This...astounds me, honestly.

I guess full kudos to @aerorahul for using, uh... -fp-model source maybe? Not sure how or why but...nice!

[bensonr]

@bensonr <https://github.com/bensonr> - Any way you'd want to support it--different .h or .inc or .F90 files--would be fine by me! I can definitely work with you to make sure things are in a good Doxygen-y way and have the right MAPL/GEOS numbers.

@mathomp4 - feel free to append a file with your list of constants here or send it to my email and I'll work to incorporate them into FMS for a future release.

[mathomp4]

@bensonr - Okay. I think this is a stripped down file:
constants-geos-like-noaa.F90.txt

You can see my ifdef version here:
https://github.com/GEOS-ESM/FMS/blob/geos/2021.03/constants/constants.F90

I tried to clean things out so the attached file would just be pretty simple in comparison.

ETA: And, of course, I didn't change any of the big Doxygen text. This would of course have to change in reality. 😄

[bensonr]

@mathomp4 - Sorry for the delay in getting to this, but can you look over PR #929 and let me know if this works for you and the team at NASA

[mathomp4]

@mathomp4 - Sorry for the delay in getting to this, but can you look over PR #929 and let me know if this works for you and the team at NASA

@bensonr Ooh. Thanks for this! I'll take a look Monday when I have some time to be precise. I do have a test model where I'm using our fork of FMS in Baselibs and I think at the moment it only has two differences, the constants and fPIC (see #806):

2021.04...GEOS-ESM:geos/2021.04

And it's zero-diff. So if I can test this out, then we might be almost there!

[bensonr]

@mathomp4 - Once we get through making sure this works for you and your team, you should feel free to put in a PR to have PIC be an option in CMakeLists.txt.

[mathomp4]

@mathomp4 - Once we get through making sure this works for you and your team, you should feel free to put in a PR to have PIC be an option in CMakeLists.txt.

@bensonr I have a draft PR here: #930

I'll undraft it next week when I can do a build. I can't imagine I screwed it up as it's pretty brain-dead CMake code, but I want to be able to check all your boxes correctly. (Unless the CI builds effectively do this?)

Of course, if any edits are needed, please let me know.

[mathomp4]

@bensonr Welp, I took your branch, built a Baselibs with it using:

-D32BIT=ON -D64BIT=ON -DCONSTANTS=GEOS

(and adding set(CMAKE_POSITION_INDEPENDENT_CODE ON)), built GEOS and...zero-diff! That actually surprised me as I thought perhaps your GEOS Constants might have been slightly different due to precision, but nope! So looks good to me!

(NOTE: Annoyingly we might soon be slightly changing a few constants in GEOS/MAPL to better match CODATA 2018. I suppose the first thing is for the GEOS constants to get in FMS, then make a PR to update them.)

[bensonr]

@mathomp4 - thanks for he review. You've got the right procedure for updates once we get things into main. If you have the changes identified and ready now, feel free to clone my branch make the updates and put in a PR. That way your updates will get in the first time and won't be delayed until a future release.

[mathomp4]

@bensonr The problem is those updates would be non-zero-diff. Frankly, no one but me is agitating for them, so once we have a long-term non-zero-diff update coming, then I'll make the PR.

For now, if I can present an FMS update that is zero-diff and still moves us from 2019-era FMS to 2021/2022...I'm fine with having some annoyance on my side later! 😄

[bensonr]

@mathomp4 - wfm