NOAA-EMC / RDASApp

Regional DAS
GNU Lesser General Public License v2.1
1 stars 8 forks source link

Clean up executable assignment in RDASApp/rrfs-test/CMakeLists.txt #79

Closed SamuelDegelia-NOAA closed 2 months ago

SamuelDegelia-NOAA commented 3 months ago

I am planning to come up with a compact way to deal with the increasingly long list of executables in RDASApp/rrfs-test/CMakeLists.txt.

SamuelDegelia-NOAA commented 3 months ago

CMake doesn't directly support dictionary data structures but you can get around it by using a list and then splitting elements via regex. @TingLei-NOAA, is the this the sort of solution you were looking for?

Instead of:

      # JEDI test names (yaml files)
      list( APPEND rrfs_fv3jedi_tests
             rrfs_fv3jedi_hyb_2022052619
             rrfs_fv3jedi_letkf_2022052619
          )

      # JEDI executables for each test case above
      list ( APPEND rrfs_fv3jedi_exes
             fv3jedi_var.x
             fv3jedi_letkf.x
      )

We can combine both lists into a more compact "dict-like" structure as in:

  set( rrfs_fv3jedi_tests
       "rrfs_fv3jedi_hyb_2022052619=fv3jedi_var.x"
       "rrfs_fv3jedi_letkf_2022052619=fv3jedi_letkf.x"
  )

And then we can retrieve the keys/pairs through:

      foreach(keypair ${rrfs_fv3jedi_tests})
         string(REGEX MATCH "([^=]+)=([^=]*)" _ ${keypair})
         set(case ${CMAKE_MATCH_1})
         set(exe ${CMAKE_MATCH_2})
      endforeach()
TingLei-NOAA commented 3 months ago

@SamuelDegelia-NOAA Sure. this version would make this step more readable and less prone to errors. But, i would prefer a more "generic"/friendlier interface with a cost of a few additional functions. Would you please have a look at chatgpt "product" : chatgpt-dictionary-mimic.sh in hera:/home/Ting.Lei/dr-out/. And let you know what your opinions are.

SamuelDegelia-NOAA commented 3 months ago

@TingLei-NOAA Yes that way looks good too and more user-friendly. BTW, what kind of message do you type in to ChatGPT to get something like this?

TingLei-NOAA commented 3 months ago

@SamuelDegelia-NOAA Interesting question. I checked the records:). My prompts were as 1). In cmake, how to create a 'dictionary like " structure , Xarray, so I can add/attached new elment like XX as a key to it and the corresponding value : YY and, then, in the future, I could get the value by ,say , Xarray{XX}. Thanks> chatgpt gave some results 2)can you add a new function, if the element XX already exists in Xarray , the code would check to see if the same value is assigned, if not, exit with error message? then, basically, chatgpt gave the scripts you looked at.

SamuelDegelia-NOAA commented 3 months ago

@TingLei-NOAA Gotcha, that is helpful. I also like how you thanked ChatGPT, hah!

guoqing-noaa commented 3 months ago

Both are great solutions!

I liked @SamuelDegelia-NOAA's method. It is straightforward and easy to read.

SamuelDegelia-NOAA commented 3 months ago

I think I slightly prefer the method @TingLei-NOAA got from ChatGPT since the lines to add a CTest are a little easier to look at (at least in my opinion). I have a version of this that builds successfully, I just need to run the CTests to confirm it works and then will put up a PR.

### Define CTests here ###

# FV3-JEDI tests
set(rrfs_fv3jedi_tests "")
add_to_dictionary(rrfs_fv3jedi_tests "rrfs_fv3jedi_hyb_2022052619"     "fv3jedi_var.x")
add_to_dictionary(rrfs_fv3jedi_tests "rrfs_fv3jedi_letkf_2022052619"   "fv3jedi_letkf.x")

# MPAS-JEDI tests
set(rrfs_mpasjedi_tests "")
add_to_dictionary(rrfs_mpasjedi_tests "rrfs_mpasjedi_2022052619_Ens3Dvar"     "mpasjedi_variational.x")
add_to_dictionary(rrfs_mpasjedi_tests "rrfs_mpasjedi_2022052619_atms_npp_qc"  "mpasjedi_variational.x")
add_to_dictionary(rrfs_mpasjedi_tests "rrfs_mpasjedi_2022052619_letkf"        "mpasjedi_enkf.x")
guoqing-noaa commented 3 months ago

Either one works for me at the moment.

FYI, are you aware that GDASApp now builds one single executable for all tests? I think this may be our final solution:

issue: https://github.com/NOAA-EMC/GDASApp/issues/1085 PR: https://github.com/NOAA-EMC/GDASApp/pull/1075 (the associated global workflow PR https://github.com/NOAA-EMC/global-workflow/pull/2565#pullrequestreview-2036711012)

SamuelDegelia-NOAA commented 3 months ago

Good point @guoqing-noaa. We actually discussed this option just yesterday. I think the mega executable route will be necessary eventually no matter what per NCO compliance rules.

But even if the CTests all use the same executable, we will still need an argument to specify the run type such as:

./rdas_jedi.x fv3 letkf config.yaml.

So getting this "dictionary-like" structure for CMake will be useful in the future.

guoqing-noaa commented 3 months ago

If we want to go this route (one single exe), I think the earlier the better.

Yes, I agree that the "dictionary-like" feature is good to have. We can add them when needed.

SamuelDegelia-NOAA commented 3 months ago

We can discuss this at our next meeting on Tuesday but I agree that it is probably better to work on this sooner rather than later.

TingLei-NOAA commented 3 months ago

@SamuelDegelia-NOAA Great. You has already had a working version using dictionary-like structure. I would recommend a PR for this.

SamuelDegelia-NOAA commented 2 months ago

Closed per PR #81.