Deal with uninitialized variables in MOSART_heat_mod.F90 #5343

liho745 · 2022-12-01T17:36:10Z

The use of uninitialized variables (i.e., Qsur and Qsub) in the computation of the change of energy
due to heat exchange with the environment and advection is fixed. Currently, it is assumed that those
changes in energy are zero.

Fixes #5332
[BFB]

ambrad

This resolves #5332. I don't have any knowledge of the meaning of the model changes, but my understanding is that these changes do not affect E3SM productions configurations. That is, this is a BFB PR from the perspective of E3SM, right?

liho745 · 2022-12-01T17:47:33Z

This resolves #5332. I don't have any knowledge of the meaning of the model changes, but my understanding is that these changes do not affect E3SM productions configurations. That is, this is a BFB PR from the perspective of E3SM, right?

I'd say yes based on my understanding, but I have not done any BFB test.

ambrad

I'll approve based on resolution of the issue. I expect this PR to be BFB. @bishtgautam, I suggest you run e3sm_developer on, e.g., Chrysalis to be sure that it is indeed BFB before merging it to next.

bishtgautam

Minor code clean up changes requested.

components/mosart/src/riverroute/MOSART_heat_mod.F90

bishtgautam · 2022-12-01T18:07:59Z

@ambrad Typically, the developer is responsible for running the regression test suite and ensuring the test suite's results are as expected.

liho745 · 2022-12-02T03:22:03Z

@ambrad Typically, the developer is responsible for running the regression test suite and ensuring the test suite's results are as expected.

I have addressed the comments from @bishtgautam, and performed the E3SM land developer's tests on Compy -- all passed with BFB.

bishtgautam · 2022-12-02T17:44:21Z

@liho745 I have edited the first comment of this PR, which will be copied/pasted as the merge commit message. For the record, below is the original first comment

In MOSART_heat_mod.F90, Qsur and Qsub were not initialized before using them in Line 91. Lines 91 and 92 calculate deltaM_t and deltaH_t that are actually not used in the later calculations, i.e., since here a simplified approach is used to calculate the stream temperature instead of the energy balance approach. I commented off Qsur and Qsub and made another couple of minor changes. A global test run was performed with the IELM compset and r05_r05 grids, and heatflag was set to true. The simulated stream temperature seems reasonable in terms of magnitude and spatial patterns.

bishtgautam · 2022-12-02T17:45:18Z

@liho745 How about also adding a test for this stealth feature?

liho745 · 2022-12-02T17:48:16Z

@liho745 I have edited the first comment of this PR, which will be copied/pasted as the merge commit message. For the record, below is the original first comment

In MOSART_heat_mod.F90, Qsur and Qsub were not initialized before using them in Line 91. Lines 91 and 92 calculate deltaM_t and deltaH_t that are actually not used in the later calculations, i.e., since here a simplified approach is used to calculate the stream temperature instead of the energy balance approach. I commented off Qsur and Qsub and made another couple of minor changes. A global test run was performed with the IELM compset and r05_r05 grids, and heatflag was set to true. The simulated stream temperature seems reasonable in terms of magnitude and spatial patterns.

Sounds good.

liho745 · 2022-12-02T17:48:50Z

@liho745 How about also adding a test for this stealth feature?

I don't quite follow. Can you elaborate?

bishtgautam · 2022-12-02T17:54:57Z

Add a new test (say ERS.<RES>.<COMPSET>.mosart-heat) in the e3sm_mosart_developer test suite in tests.py#L13 and add a corresponding new heat directory in components/mosart/cime_config/testdefs/testmods_dirs/mosart. Does that make sense?

hydrotian · 2022-12-02T19:17:48Z

@liho745 I wrote a document about adding a new ELM test last year. Hope the information is still up to date.

liho745 · 2022-12-04T18:39:40Z

Many thanks! I have added a new test for MOSART-heat, and passed the baseline tests BFB.

bishtgautam · 2022-12-05T20:19:25Z

cime_config/tests.py

@@ -13,13 +13,21 @@
    "e3sm_mosart_developer" : {
        "share" : True,
        "time"  : "0:45:00",
+        "inherit" : ("e3sm_mosart_heat"),


@liho745 Thanks for adding a new test. Instead of creating a new test suite (i.e. e3sm_mosart_heat), please directly add the new test in the e3sm_mosart_developer test suite at https://github.com/E3SM-Project/E3SM/pull/5343/files#diff-e2fbf220fd98012c3bdf61d7f341a1815f43823af2ffd3bf926d5aafc4363d8bR20. Does it make sense?

Actually not quite -- I followed the example of the sediment test added by Donghui Xu, where he created a mosart-sediment separately and then inherited it in the e3sm_mosart_developer suite. Are there any considerations to treating sediment and heat differently in the testing?

I will have to revisit the approach taken for the sediment test when I re-merge that PR into next. To avoid creating many new test suites, directly add ERS.r05_r05.RMOSGPCC.mosart-heat in the e3sm_mosart_developer test suite.

It has to be created as a sub-suite and then called from the main e3sm_mosart_developer suite, since in this mosart-heat test we are turning on heatflag which is set to .false. by default in the other test cases. I have discussed this with Donghui about the reason he created a mosart-sediment test as a sub-suite, and I quote from him "mosart-sediment cannot run with other test cases with the same build". Similarly, in my case, if I create a mosart-heat case directly within e3sm_mosart_developer suite, the tests will just fail for mosart-heat.

It seems at least more discussion is needed.

I quote from him "mosart-sediment cannot run with other test cases with the same build".

Okay, I now recall this issue with mosart sediment. Does your mosart heat test need different build flags (not namelist flags) than those tests in the e3sm_mosart_developer test suite?

Yes, I have to turn on heatflag via user_nl_mosart.

That is a namelist flag and not a build flag (e.g. -DUSE_PETSC_LIB), which is used when compiling various *.F90. Thus, you should be able to directly add the new test in the e3sm_mosart_developer test suite.

That is a namelist flag and not a build flag (e.g. -DUSE_PETSC_LIB), which is used when compiling various *.F90. Thus, you should be able to directly add the new test in the e3sm_mosart_developer test suite.

I tried what you said, and the test results are below, i.e., the baseline test failed.

ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat (Overall: DIFF) details:
PASS ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat CREATE_NEWCASE
PASS ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat XML
PASS ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat SETUP
PASS ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat SHAREDLIB_BUILD time=277
FAIL ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat NLCOMP
PASS ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat MODEL_BUILD time=212
PASS ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat SUBMIT
PASS ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat RUN time=51
PASS ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat COMPARE_base_rest
FAIL ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat BASELINE 21ffb4d: ERROR BFAIL baseline directory '/compyfs/liho623/e3sm_scratch/e3sm_baseline/21ffb4d/ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat' does not exist
PASS ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat MEMLEAK
PASS ERS.r05_r05.RMOSGPCC.compy_intel.mosart-heat SHORT_TERM_ARCHIVER

The above failure is okay because the baseline for this new test doesn't exist.

Sounds good. Added the mosart-heat test directly into the e3sm_mosart_developer suite.

The use of uninitialized variables (i.e., `Qsur` and `Qsub`) in the computation of the change of energy due to heat exchange with the environment and advection is fixed. Currently, it is assumed that those changes in energy are zero. Fixes #5332 [BFB]

bishtgautam · 2022-12-13T19:11:15Z

Merged to next

deal with uninitialized variables in MOSART_heat_mod.F90

a8c0733

liho745 requested review from hydrotian, bishtgautam and ambrad December 1, 2022 17:36

ambrad reviewed Dec 1, 2022

View reviewed changes

rljacob assigned bishtgautam Dec 1, 2022

rljacob removed the request for review from bishtgautam December 1, 2022 17:51

ambrad approved these changes Dec 1, 2022

View reviewed changes

bishtgautam requested changes Dec 1, 2022

View reviewed changes

components/mosart/src/riverroute/MOSART_heat_mod.F90 Outdated Show resolved Hide resolved

components/mosart/src/riverroute/MOSART_heat_mod.F90 Outdated Show resolved Hide resolved

components/mosart/src/riverroute/MOSART_heat_mod.F90 Outdated Show resolved Hide resolved

bishtgautam added River bug fix PR labels Dec 1, 2022

rljacob added this to the v2.1rc milestone Dec 1, 2022

Addressing a few minor formatting issues

a8d2a3a

bishtgautam self-requested a review December 2, 2022 17:32

bishtgautam approved these changes Dec 2, 2022

View reviewed changes

hydrotian approved these changes Dec 2, 2022

View reviewed changes

adding a new test for mosart-heat

ff3f377

bishtgautam reviewed Dec 5, 2022

View reviewed changes

adding a mosart-heat test directly into the e3sm_mosart_developer suite

95a4541

bishtgautam merged commit 0273cfa into master Dec 14, 2022

bishtgautam deleted the hongyili/mosart/mosart_heat_v3 branch December 14, 2022 20:01

xylar mentioned this pull request Apr 13, 2023

Standalone MPAS-Ocean builds broken #5598

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deal with uninitialized variables in MOSART_heat_mod.F90 #5343

Deal with uninitialized variables in MOSART_heat_mod.F90 #5343

liho745 commented Dec 1, 2022 •

edited by bishtgautam

Loading

ambrad left a comment

liho745 commented Dec 1, 2022

ambrad left a comment

bishtgautam left a comment

bishtgautam commented Dec 1, 2022

liho745 commented Dec 2, 2022

bishtgautam commented Dec 2, 2022

bishtgautam commented Dec 2, 2022

liho745 commented Dec 2, 2022

liho745 commented Dec 2, 2022

bishtgautam commented Dec 2, 2022

hydrotian commented Dec 2, 2022 •

edited

Loading

liho745 commented Dec 4, 2022

bishtgautam Dec 5, 2022 •

edited

Loading

liho745 Dec 5, 2022

bishtgautam Dec 7, 2022

liho745 Dec 7, 2022 •

edited

Loading

bishtgautam Dec 7, 2022

liho745 Dec 7, 2022

bishtgautam Dec 7, 2022

liho745 Dec 8, 2022 •

edited

Loading

bishtgautam Dec 8, 2022

liho745 Dec 13, 2022

bishtgautam commented Dec 13, 2022

Deal with uninitialized variables in MOSART_heat_mod.F90 #5343

Deal with uninitialized variables in MOSART_heat_mod.F90 #5343

Conversation

liho745 commented Dec 1, 2022 • edited by bishtgautam Loading

ambrad left a comment

Choose a reason for hiding this comment

liho745 commented Dec 1, 2022

ambrad left a comment

Choose a reason for hiding this comment

bishtgautam left a comment

Choose a reason for hiding this comment

bishtgautam commented Dec 1, 2022

liho745 commented Dec 2, 2022

bishtgautam commented Dec 2, 2022

bishtgautam commented Dec 2, 2022

liho745 commented Dec 2, 2022

liho745 commented Dec 2, 2022

bishtgautam commented Dec 2, 2022

hydrotian commented Dec 2, 2022 • edited Loading

liho745 commented Dec 4, 2022

bishtgautam Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

liho745 Dec 5, 2022

Choose a reason for hiding this comment

bishtgautam Dec 7, 2022

Choose a reason for hiding this comment

liho745 Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

bishtgautam Dec 7, 2022

Choose a reason for hiding this comment

liho745 Dec 7, 2022

Choose a reason for hiding this comment

bishtgautam Dec 7, 2022

Choose a reason for hiding this comment

liho745 Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

bishtgautam Dec 8, 2022

Choose a reason for hiding this comment

liho745 Dec 13, 2022

Choose a reason for hiding this comment

bishtgautam commented Dec 13, 2022

liho745 commented Dec 1, 2022 •

edited by bishtgautam

Loading

hydrotian commented Dec 2, 2022 •

edited

Loading

bishtgautam Dec 5, 2022 •

edited

Loading

liho745 Dec 7, 2022 •

edited

Loading

liho745 Dec 8, 2022 •

edited

Loading