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

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

Closed
SamuelDegelia-NOAA opened this issue Jun 13, 2024 · 13 comments
Closed
Assignees

Comments

@SamuelDegelia-NOAA
Copy link
Contributor

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 SamuelDegelia-NOAA self-assigned this Jun 13, 2024
@SamuelDegelia-NOAA
Copy link
Contributor Author

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
Copy link
Contributor

TingLei-NOAA commented Jun 14, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor

TingLei-NOAA commented Jun 14, 2024

@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
Copy link
Contributor Author

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

@guoqing-noaa
Copy link
Collaborator

Both are great solutions!

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

@SamuelDegelia-NOAA
Copy link
Contributor Author

SamuelDegelia-NOAA commented Jun 14, 2024

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
Copy link
Collaborator

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: NOAA-EMC/GDASApp#1085
PR: NOAA-EMC/GDASApp#1075
(the associated global workflow PR NOAA-EMC/global-workflow#2565 (review))

@SamuelDegelia-NOAA
Copy link
Contributor Author

SamuelDegelia-NOAA commented Jun 14, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

SamuelDegelia-NOAA commented Jun 14, 2024

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
Copy link
Contributor

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

@SamuelDegelia-NOAA
Copy link
Contributor Author

Closed per PR #81.

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

No branches or pull requests

3 participants