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

Set BUILD_MGBF default to OFF #765

Open
RussTreadon-NOAA opened this issue Jul 2, 2024 · 14 comments · May be fixed by #772
Open

Set BUILD_MGBF default to OFF #765

RussTreadon-NOAA opened this issue Jul 2, 2024 · 14 comments · May be fixed by #772
Assignees

Comments

@RussTreadon-NOAA
Copy link
Contributor

MGBF is not currently used in gsi.x. However, MGBF compilation is on by default in CMakeLists.txt.

option(BUILD_GSDCLOUD "Build GSD Cloud Analysis Library" OFF)
option(BUILD_MGBF "Build MGBF Library" ON)

Setting the MGBF build option to off aligns with the GSD Cloud Analysis Library.

This issue is opened to change the MGBF default to OFF

@RussTreadon-NOAA
Copy link
Contributor Author

@ShunLiu-NOAA , @hu5970 , and @ManuelPondeca-NOAA : Are we OK with setting the default MGBF build option to OFF?

@ShunLiu-NOAA
Copy link
Contributor

@RussTreadon-NOAA it is OK for all RRFS test now.

@TingLei-NOAA
Copy link
Contributor

@RussTreadon-NOAA Turning off the MGBF option , currently, would cause the building of GSI fail, because, unfortunately, the mgbf codes in gsi are not coordinated with the compiler option as the GSDCLOUD subroutines in src/gsi (like gsdcloudanalysis.F90 the "ifdef RR_CLOUNDANALYSIS" block ) does. So, A PR for this issue would need to include adding similar treatments (but more efforts needed for the smaller pieces of mgbf codes in hybrid_ensemble_isotropic.F90) for mgbf codes in gsi.

@RussTreadon-NOAA
Copy link
Contributor Author

Thank you @TingLei-NOAA for explaining that setting BUILD_MGBF to OFF would cause the GSI build to fail. I did not realize this. This is unfortunate.

We should not have allowed PR #700 to be merged into develop as is MGBF's addition to the GSI should have followed the approach used by the GSD cloud analysis. GSI realizations which need the GSD cloud analysis set BUILD_GSDCLOUD on. The default is off. The same should be true for MGBF.

GFS v16 does not and GFS v17 will not use MGBF. However, we must build MGBF as part of the global GSI.

@TingLei-NOAA
Copy link
Contributor

@RussTreadon-NOAA Your points are completely understood.
If you and other colleagues agree, I could make a draft pr to do the similar treatment to mgbf codes in gsi to allow the mgbf option to be turned off as GSDCLOUD. My concern for such a treatment for mgbf is those mgbf codes are scattered and might cause the treatments using directives looks not clean.
Please let me know if we should explore this change.

@RussTreadon-NOAA
Copy link
Contributor Author

I don't know the MGBF code so I don't know how extensive the changes may be. Thus, a draft PR would be useful. That said, do not read this comment as a request to prepare a draft PR. I am not tasking anyone to do anything. Please discuss among staff the appropriate action(s) to take.

@TingLei-NOAA
Copy link
Contributor

@RussTreadon-NOAA Thanks for your inputs! I will keep you updated of what is going on with this topic!

@TingLei-NOAA
Copy link
Contributor

@RussTreadon-NOAA @ShunLiu-NOAA , we have a draft PR to show the changes done for MGBF to be able to be turned off. Please let us know if that changes in that draft PR are regarded as the solution for this PR and could proceed to the PR process, or a better solution could be sought.

@RussTreadon-NOAA
Copy link
Contributor Author

Thank you @TingLei-NOAA for making these changes. The changes look straightforward. Do the changes yield the desired behavior

  1. successfully build and run ctests without MGBF (default)
  2. successfully build and run with MGBF when USE_MGBF=ON

1 is easily tested. 2 requires you to run a test which exercises MGBF.

@TingLei-NOAA
Copy link
Contributor

@RussTreadon-NOAA The current regression tests are being run with MGBF turned off.
A MGBF test as discussed in another thread is being discussed and being prepared but might need some time .

@RussTreadon-NOAA
Copy link
Contributor Author

@RussTreadon-NOAA The current regression tests are being run with MGBF turned off. A MGBF test as discussed in another thread is being discussed and being prepared but might need some time .

Yes, it is important to add a MGBF ctest if we intend to use MGBF via an operational realization of the GSI. Until this test is available, you should run an offline test to confirm that 2 is valid.

@RussTreadon-NOAA
Copy link
Contributor Author

@TingLei-NOAA , what is the status of this issue?

@TingLei-NOAA
Copy link
Contributor

@RussTreadon-NOAA I had put the corresponding PR to be on "draft", while ,as I reported , using the current compiler on wcoss2,,that PR could fail the ctests even when the option is on. I will request an extension to keep this issue open and I will spend more time to work on it.

@RussTreadon-NOAA
Copy link
Contributor Author

@TingLei-NOAA , thanks for the update.

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

Successfully merging a pull request may close this issue.

3 participants