Add regression tests with MULTI_GASES #112

XiaqiongZhou-NOAA · 2020-04-28T16:59:33Z

To add regression tests with MULTI_GASES=Y

Related pull requests:
NOAA-EMC/fv3atm#108
NOAA-EMC/GFDL_atmos_cubed_sphere#19
NCAR/ccpp-physics#444

The input data with C96L149 saved temporarily at /gpfs/hps3/emc/ensemble/noscrub/Xiaqiong.Zhou/RT/NEMSfv3gfs (WCOSS_CRAY)
and /scratch2/NCEPDEV/ensemble/Xiaqiong.Zhou/noscrub/RT/NEMSfv3gfs (HERA)

climbfuji · 2020-07-01T14:27:48Z

While this implementation passes the regression tests, it is not correct and won't work with the old gnumake build.

cmake: These entries - same for all machines - say that MULTI_GASES is on by default:

heinzeller-lt:ufs-weather-model-rt_multigases-20200701 dom.heinzeller$ cat cmake/configure_hera.intel.cmake
...
option(MULTI_GASES  "Enable MULTI_GASES" ON)
...

Then, for each compiler

heinzeller-lt:ufs-weather-model-rt_multigases-20200701 dom.heinzeller$ cat cmake/Intel.cmake
...
if(MULTI_GASES)
    add_definitions(-DMULTI_GASES)
endif()
...

With this, -DMULTI_GASES should always be passed to the build. However, in tests/compile_cmake.sh

if [[ "${MAKE_OPT}" == *"CCPP=Y"* ]]; then
...
  if [[ "${MAKE_OPT}" == *"MULTI_GASES=Y"* ]]; then
    CCPP_CMAKE_FLAGS="${CCPP_CMAKE_FLAGS} -DMULTI_GASES=ON"
  else
    CCPP_CMAKE_FLAGS="${CCPP_CMAKE_FLAGS} -DMULTI_GASES=OFF"
  fi
...

hence the switch to turn off multi gases is in compile_cmake.sh - and only for CCPP! That means your PR will build IPD with MULTI_GASES on no matter what when cmake is used.

For the gnu make build, there are no default values set in conf/configure.fv3.*, but each of them has

ifeq ($(MULTI_GASES),Y)
CPPDEFS += -DMULTI_GASES

While not correct, this at least means that by default the gnumake build turns multi gases off. Each conf/configure.fv3.* should have a default line towards the top saying

MULTI_GASES = N

DusanJovic-NOAA · 2020-07-01T14:50:07Z

While this implementation passes the regression tests, it is not correct and won't work with the old gnumake build.

cmake: These entries - same for all machines - say that MULTI_GASES is on by default:
heinzeller-lt:ufs-weather-model-rt_multigases-20200701 dom.heinzeller$ cat cmake/configure_hera.intel.cmake
...
option(MULTI_GASES  "Enable MULTI_GASES" ON)
...
Then, for each compiler
heinzeller-lt:ufs-weather-model-rt_multigases-20200701 dom.heinzeller$ cat cmake/Intel.cmake
...
if(MULTI_GASES)
    add_definitions(-DMULTI_GASES)
endif()
...
With this, -DMULTI_GASES should always be passed to the build. However, in tests/compile_cmake.sh
if [[ "${MAKE_OPT}" == *"CCPP=Y"* ]]; then
...
  if [[ "${MAKE_OPT}" == *"MULTI_GASES=Y"* ]]; then
    CCPP_CMAKE_FLAGS="${CCPP_CMAKE_FLAGS} -DMULTI_GASES=ON"
  else
    CCPP_CMAKE_FLAGS="${CCPP_CMAKE_FLAGS} -DMULTI_GASES=OFF"
  fi
...
hence the switch to turn off multi gases is in compile_cmake.sh - and only for CCPP! That means your PR will build IPD with MULTI_GASES on no matter what when cmake is used.

For the gnu make build, there are no default values set in conf/configure.fv3.*, but each of them has
ifeq ($(MULTI_GASES),Y)
CPPDEFS += -DMULTI_GASES
While not correct, this at least means that by default the gnumake build turns multi gases off. Each conf/configure.fv3.* should have a default line towards the top saying
MULTI_GASES = N

MULTI_GASES should be set to OFF in all `cmake/configure_*.cmake

Why is this even compile time option, is there any disadvantage using run-time flag (namelist) which will be set to .false. by default. We should limit number of build time options. It complicates the build system, it complicates regression testing and makes source code less readable.

DusanJovic-NOAA · 2020-07-01T14:58:20Z

That means your PR will build IPD with MULTI_GASES on no matter what when cmake is used.

We do not build nor test IPD in develop branch anymore. We stopped doing that on May 1. It's been two months since then. We should remove IPD code in one of the future commits.

climbfuji · 2020-07-01T15:01:26Z

That means your PR will build IPD with MULTI_GASES on no matter what when cmake is used.

We do not build nor test IPD in develop branch anymore. We stopped doing that on May 1. It's been two months since then. We should remove IPD code in one of the future commits.

This commit even adds an IPD regression test - should we allow this?

junwang-noaa · 2020-07-01T15:12:42Z

Dusan, I am updating GFSv16 test case with CCPP, since GFSv16 is using IPD only, I do hope we can at least support IPD functionality until this work is done, so we are confident that when global RT with GFSv16 is running with CCPP, it reproduces IPD in repro mode, then we move on with CCPP. On the other side, if CCPP group or other people are willing to do evaluation of CCPP test against FV3 parallel, I am OK to drop off IPD functionality at this time too. Jun

…

On Wed, Jul 1, 2020 at 11:02 AM Dom Heinzeller ***@***.***> wrote: *That means your PR will build IPD with MULTI_GASES on no matter what when cmake is used.* We do not build nor test IPD in develop branch anymore. We stopped doing that on May 1. It's been two months since then. We should remove IPD code in one of the future commits. This commit even adds an IPD regression test - should we allow this? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#112 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI7D6TK6TXOJ6YWAOBIRNZDRZNFVPANCNFSM4MTAB5OQ> .

junwang-noaa · 2020-07-01T15:13:45Z

Please do not add any IPD test in RT, we just dropped them in early May. On Wed, Jul 1, 2020 at 11:12 AM Jun Wang - NOAA Federal <[email protected]> wrote:

…

Dusan, I am updating GFSv16 test case with CCPP, since GFSv16 is using IPD only, I do hope we can at least support IPD functionality until this work is done, so we are confident that when global RT with GFSv16 is running with CCPP, it reproduces IPD in repro mode, then we move on with CCPP. On the other side, if CCPP group or other people are willing to do evaluation of CCPP test against FV3 parallel, I am OK to drop off IPD functionality at this time too. Jun On Wed, Jul 1, 2020 at 11:02 AM Dom Heinzeller ***@***.***> wrote: > *That means your PR will build IPD with MULTI_GASES on no matter what > when cmake is used.* > > We do not build nor test IPD in develop branch anymore. We stopped doing > that on May 1. It's been two months since then. We should remove IPD code > in one of the future commits. > > This commit even adds an IPD regression test - should we allow this? > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#112 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AI7D6TK6TXOJ6YWAOBIRNZDRZNFVPANCNFSM4MTAB5OQ> > . >

SMoorthi-emc · 2020-07-01T15:16:07Z

I urge not to drop IPD, as it is very hard, if not almost impossible to debug with CCPP. Having IPD option makes it a lot easier to debug. My 2 cents. Thanks Moorthi

…

On Wed, Jul 1, 2020 at 11:13 AM Jun Wang ***@***.***> wrote: Dusan, I am updating GFSv16 test case with CCPP, since GFSv16 is using IPD only, I do hope we can at least support IPD functionality until this work is done, so we are confident that when global RT with GFSv16 is running with CCPP, it reproduces IPD in repro mode, then we move on with CCPP. On the other side, if CCPP group or other people are willing to do evaluation of CCPP test against FV3 parallel, I am OK to drop off IPD functionality at this time too. Jun On Wed, Jul 1, 2020 at 11:02 AM Dom Heinzeller ***@***.***> wrote: > *That means your PR will build IPD with MULTI_GASES on no matter what when > cmake is used.* > > We do not build nor test IPD in develop branch anymore. We stopped doing > that on May 1. It's been two months since then. We should remove IPD code > in one of the future commits. > > This commit even adds an IPD regression test - should we allow this? > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > < #112 (comment) >, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AI7D6TK6TXOJ6YWAOBIRNZDRZNFVPANCNFSM4MTAB5OQ > > . > — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#112 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALLVRYT2I5ZCSS2FEQBJGYDRZNG7ZANCNFSM4MTAB5OQ> .

-- Dr. Shrinivas Moorthi Research Meteorologist Modeling and Data Assimilation Branch Environmental Modeling Center / National Centers for Environmental Prediction 5830 University Research Court - (W/NP23), College Park MD 20740 USA Tel: (301)683-3718 e-mail: [email protected] Phone: (301) 683-3718 Fax: (301) 683-3718

climbfuji · 2020-07-01T15:26:03Z

I urge not to drop IPD, as it is very hard, if not almost impossible to debug with CCPP. Having IPD option makes it a lot easier to debug. My 2 cents. Thanks Moorthi
…

Moorthi, I hope you can see how difficult it is to maintain two systems in parallel - we have been doing this for so long and it has taken so much time that we could have invested better in addressing your concerns. If you can describe in detail where your specific problems are, then we can hopefully work towards a solution that helps you do what you need to do. We are not running IPD regression tests anymore, and we will not turn them back on. There is no guarantee that it will work in the future, and as this commit shows it would have "broken" the IPD build if I didn't look carefully.

XiaqiongZhou-NOAA · 2020-07-01T15:29:45Z

IPD multi_gases is not added in regression test, but it should be available for the test purpose as Henry mentioned that they still need it.

XiaqiongZhou-NOAA · 2020-07-01T15:32:04Z

Dom:
I will add MULTI_GASES = N in conf/configure.fv3.* . What else should I changed?

DusanJovic-NOAA · 2020-07-01T15:35:05Z

That means your PR will build IPD with MULTI_GASES on no matter what when cmake is used.
We do not build nor test IPD in develop branch anymore. We stopped doing that on May 1. It's been two months since then. We should remove IPD code in one of the future commits.

This commit even adds an IPD regression test - should we allow this?

NO!

climbfuji · 2020-07-01T15:45:43Z

Dom:
I will add MULTI_GASES = N in conf/configure.fv3.* . What else should I changed?

If you are making changes to this PR, then you need to do the following:

add MULTI_GASES = N to the default section for each conf/configure.fv3.*
change the option MULT_GASES from ON to OFF for each cmake/configure_*.cmake
in tests/compile_cmake.sh, move the block

  if [[ "${MAKE_OPT}" == *"MULTI_GASES=Y"* ]]; then
    CCPP_CMAKE_FLAGS="${CCPP_CMAKE_FLAGS} -DMULTI_GASES=ON"
  else
    CCPP_CMAKE_FLAGS="${CCPP_CMAKE_FLAGS} -DMULTI_GASES=OFF"
  fi

outside of the block

if [[ "${MAKE_OPT}" == *"CCPP=Y"* ]]; then
  ...
fi

This way, all tests should pass against your new baseline.

SMoorthi-emc · 2020-07-01T15:46:45Z

Dom, This morning I was trying to debug something and I realized that it was almost impossible under CCPP unless I go in and hack the code.All the debug prints and logicals have been removed. So, I decided to go back to IPD to do my debugging first. I am not saying that regression tests need to be maintained. Even if you all delete IPD from the source, I will maintain my branch as it is too valuable for me. Thanks Moorthi

…

On Wed, Jul 1, 2020 at 11:26 AM Dom Heinzeller ***@***.***> wrote: I urge not to drop IPD, as it is very hard, if not almost impossible to debug with CCPP. Having IPD option makes it a lot easier to debug. My 2 cents. Thanks Moorthi … <#m_388869045140375181_> Moorthi, I hope you can see how difficult it is to maintain two systems in parallel - we have been doing this for so long and it has taken so much time that we could have invested better in addressing your concerns. If you can describe in detail where your specific problems are, then we can hopefully work towards a solution that helps you do what you need to do. We are not running IPD regression tests anymore, and we will not turn them back on. There is no guarantee that it will work in the future, and as this commit shows it would have "broken" the IPD build if I didn't look carefully. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#112 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALLVRYRVJ2HJCJCNM3KZVELRZNIRVANCNFSM4MTAB5OQ> .

-- Dr. Shrinivas Moorthi Research Meteorologist Modeling and Data Assimilation Branch Environmental Modeling Center / National Centers for Environmental Prediction 5830 University Research Court - (W/NP23), College Park MD 20740 USA Tel: (301)683-3718 e-mail: [email protected] Phone: (301) 683-3718 Fax: (301) 683-3718

junwang-noaa

Looks good.

* See ufs-community#43 ufs-community#34 ufs-community#31

XiaqiongZhou-NOAA added 6 commits March 30, 2020 16:58

Add the option to run regression test with MULTI_GASES=Y

3fd9733

Add multi_gases CCPP regression test

1577985

merge ufs-community/ufs

c9ae4d7

point to FV3 right revision number

bb8158f

point to FV3 right revision number

5d132c3

add CCPP Multi_gases regression tests in the list

0f605e9

This was referenced Apr 28, 2020

Add regression tests with MULTI_GASES NOAA-EMC/fv3atm#108

Merged

Fixes to run 32BIT MULTI_GASES NOAA-GFDL/GFDL_atmos_cubed_sphere#37

Closed

Fixes to run the CCPP multi_gases option with 32 bit NCAR/ccpp-physics#444

Merged

XiaqiongZhou-NOAA marked this pull request as ready for review April 28, 2020 17:17

XiaqiongZhou-NOAA mentioned this pull request Apr 28, 2020

Fix for multi_gases to 32 bit compiling NOAA-EMC/GFDL_atmos_cubed_sphere#19

Merged

XiaqiongZhou-NOAA mentioned this pull request May 14, 2020

Merge GFDL new dynamic core to EMC fork/branch NOAA-EMC/GFDL_atmos_cubed_sphere#15

Merged

XiaqiongZhou-NOAA added 9 commits June 22, 2020 17:42

Point to new submodules

d247254

Merge remote-tracking branch 'ufscomm/develop' into rt_multigases

ac72b08

Add the multi_gases compiling option for cmake

a76ac62

Add the multi_gases regression test on Orion

0b2fa23

Add the GNU test with the multi_gases opition

18aaa98

Point FV3 to a new version

f4919de

Merge remote-tracking branch 'ufscomm/develop' into rt_multigases

18df462

Point FV3 to a new version

a746f83

Change to the new baseline

31740f7

XiaqiongZhou-NOAA added 8 commits July 1, 2020 16:19

Correct configure files with MULTI_GASES option off

17ba147

Change multi_gases namelist

b89530c

Point FV3 to a new version

15dea5e

Merge remote ufscomm/develop to rt_multigases branch

5adab14

Upload log files

9e21fc7

Upload log files

56bdc47

Point FV3 to a new version

94d9f2f

Point FV3 to the public version

3f012fe

climbfuji approved these changes Jul 7, 2020

View reviewed changes

DusanJovic-NOAA requested a review from junwang-noaa July 7, 2020 14:58

junwang-noaa approved these changes Jul 7, 2020

View reviewed changes

DusanJovic-NOAA approved these changes Jul 7, 2020

View reviewed changes

DusanJovic-NOAA merged commit a7b9b6d into ufs-community:develop Jul 7, 2020

XiaSun-Atmos pushed a commit to XiaSun-Atmos/ufs-weather-model that referenced this pull request Aug 8, 2020

Bugfixes from public release (ufs-community#112)

63658ed

* See ufs-community#43 ufs-community#34 ufs-community#31

XiaqiongZhou-NOAA deleted the rt_multigases branch June 13, 2021 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add regression tests with MULTI_GASES #112

Add regression tests with MULTI_GASES #112

XiaqiongZhou-NOAA commented Apr 28, 2020 •

edited

Loading

climbfuji commented Jul 1, 2020

DusanJovic-NOAA commented Jul 1, 2020

DusanJovic-NOAA commented Jul 1, 2020

climbfuji commented Jul 1, 2020

junwang-noaa commented Jul 1, 2020 via email

junwang-noaa commented Jul 1, 2020 via email

SMoorthi-emc commented Jul 1, 2020 via email

climbfuji commented Jul 1, 2020

XiaqiongZhou-NOAA commented Jul 1, 2020

XiaqiongZhou-NOAA commented Jul 1, 2020

DusanJovic-NOAA commented Jul 1, 2020

climbfuji commented Jul 1, 2020

SMoorthi-emc commented Jul 1, 2020 via email

junwang-noaa left a comment

Add regression tests with MULTI_GASES #112

Add regression tests with MULTI_GASES #112

Conversation

XiaqiongZhou-NOAA commented Apr 28, 2020 • edited Loading

climbfuji commented Jul 1, 2020

DusanJovic-NOAA commented Jul 1, 2020

DusanJovic-NOAA commented Jul 1, 2020

climbfuji commented Jul 1, 2020

junwang-noaa commented Jul 1, 2020 via email

junwang-noaa commented Jul 1, 2020 via email

SMoorthi-emc commented Jul 1, 2020 via email

climbfuji commented Jul 1, 2020

XiaqiongZhou-NOAA commented Jul 1, 2020

XiaqiongZhou-NOAA commented Jul 1, 2020

DusanJovic-NOAA commented Jul 1, 2020

climbfuji commented Jul 1, 2020

SMoorthi-emc commented Jul 1, 2020 via email

junwang-noaa left a comment

Choose a reason for hiding this comment

XiaqiongZhou-NOAA commented Apr 28, 2020 •

edited

Loading