Correct ocean conservation check settings #6643

mark-petersen · 2024-09-24T16:29:07Z

Currently, the ocean conservation check analysis member overwrites the first entry in the file with a zero after restarts for some variables. This PR corrects this behavior so that the first day's entry in a monthly conservation check file is identical between continuous runs and a run with a restart break.

Fixes #6642

[NML] for some mpaso resolutions
[BFB] for all tested files

github-actions · 2024-09-24T16:31:08Z

PR Preview Action v1.4.8
🚀 Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6643/
on branch `gh-pages` at 2024-09-26 18:05 UTC

xylar · 2024-09-24T16:47:52Z

components/mpas-ocean/bld/namelist_files/namelist_defaults_mpaso.xml

-<config_AM_conservationCheck_enable ocn_grid="FRISwISC02to60E3r1">.true.</config_AM_conservationCheck_enable>
-<config_AM_conservationCheck_enable ocn_grid="FRISwISC01to60E3r1">.true.</config_AM_conservationCheck_enable>
-<config_AM_conservationCheck_enable ocn_grid="RRSwISC6to18E3r5">.true.</config_AM_conservationCheck_enable>
+<config_AM_conservationCheck_enable>.true.</config_AM_conservationCheck_enable>


@mark-petersen, we may want this turned off for QU240, etc. For light-weight testing.

Let's explicitly set this to .false. for:

oQU240

oQU480

oQU240wLI

oQU120

These meshes are used for testing:
https://github.com/E3SM-Project/E3SM/blob/master/cime_config/tests.py
and we don't want to needlessly increase test burden.

xylar · 2024-09-24T16:48:30Z

components/mpas-seaice/bld/namelist_files/namelist_defaults_mpassi.xml

-<config_AM_conservationCheck_enable>false</config_AM_conservationCheck_enable>
+<config_AM_conservationCheck_enable>true</config_AM_conservationCheck_enable>


This should be a separate PR, I think.

Thanks @xylar -- I agree, especially since mpas-seaice is also used for F-cases where its performance is already an issue

proteanplanet · 2024-09-24T18:43:06Z

Conservation checks in both the sea ice and ocean models should not be set as a default. It was actually a mistake that they were left on in the ocean for V3 production runs - they should have been switched off. They add significant computational expense - up to 30% for the sea ice model alone. Please remove the lines in components/mpas-seaice/bld/namelist_files/namelist_defaults_mpassi.xml from this PR. Also please consider removing this as default behavior for the ocean. Conservation checks should be used for development purposes, but once we know models are conserving after checks run for PRs, the analysis members should be kept off.

xylar · 2024-09-24T18:51:38Z

@mark-petersen, in light of @proteanplanet's comment above, which makes sense to me, can you actually turn the conservation analysis member off by default for all meshes in this PR? I think we don't want to get distracted form the main objective here, which was to fix lack of restart capability in the ocean conservation analysis member.

rljacob · 2024-09-24T19:15:41Z

Could we add a restart test that does turn on the conservation analysis?

xylar · 2024-09-24T19:17:21Z

@rljacob, that's an excellent idea but will CIME notice whether the output is BFB here? I seem to recall that ERS tests don't include output from analysis, but maybe I'm misremembering.

jonbob · 2024-09-24T19:18:05Z

@rljacob -- we can do that, though I think mpas analysis member output is ignored in the tests

xylar · 2024-09-24T19:18:15Z

@mark-petersen, are you comfortable adding a test with the conservation check on or would you like me to do it?

xylar · 2024-09-24T19:19:17Z

@jonbob, jinx!

mark-petersen · 2024-09-24T20:35:05Z

I made conservation checks default off for both sea ice and ocean, per comment by @proteanplanet above. I'm checking the restart capability now.

xylar · 2024-09-25T08:29:01Z

components/mpas-ocean/bld/namelist_files/namelist_defaults_mpaso.xml

-<config_AM_conservationCheck_compute_on_startup ocn_grid="SOwISC12to60E2r4">.true.</config_AM_conservationCheck_compute_on_startup>
-<config_AM_conservationCheck_compute_on_startup ocn_grid="ECwISC30to60E2r1">.true.</config_AM_conservationCheck_compute_on_startup>
-<config_AM_conservationCheck_compute_on_startup ocn_grid="IcoswISC30E3r5">.true.</config_AM_conservationCheck_compute_on_startup>
-<config_AM_conservationCheck_compute_on_startup ocn_grid="IcosXISC30E3r7">.true.</config_AM_conservationCheck_compute_on_startup>
-<config_AM_conservationCheck_compute_on_startup ocn_grid="FRISwISC08to60E3r1">.true.</config_AM_conservationCheck_compute_on_startup>
-<config_AM_conservationCheck_compute_on_startup ocn_grid="FRISwISC04to60E3r1">.true.</config_AM_conservationCheck_compute_on_startup>
-<config_AM_conservationCheck_compute_on_startup ocn_grid="FRISwISC02to60E3r1">.true.</config_AM_conservationCheck_compute_on_startup>
-<config_AM_conservationCheck_compute_on_startup ocn_grid="FRISwISC01to60E3r1">.true.</config_AM_conservationCheck_compute_on_startup>
-<config_AM_conservationCheck_compute_on_startup ocn_grid="RRSwISC6to18E3r5">.true.</config_AM_conservationCheck_compute_on_startup>
 <config_AM_conservationCheck_write_on_startup>.false.</config_AM_conservationCheck_write_on_startup>
-<config_AM_conservationCheck_write_on_startup ocn_grid="SOwISC12to60E2r4">.true.</config_AM_conservationCheck_write_on_startup>
-<config_AM_conservationCheck_write_on_startup ocn_grid="ECwISC30to60E2r1">.true.</config_AM_conservationCheck_write_on_startup>
-<config_AM_conservationCheck_write_on_startup ocn_grid="IcoswISC30E3r5">.true.</config_AM_conservationCheck_write_on_startup>
-<config_AM_conservationCheck_write_on_startup ocn_grid="IcosXISC30E3r7">.true.</config_AM_conservationCheck_write_on_startup>
-<config_AM_conservationCheck_write_on_startup ocn_grid="FRISwISC08to60E3r1">.true.</config_AM_conservationCheck_write_on_startup>
-<config_AM_conservationCheck_write_on_startup ocn_grid="FRISwISC04to60E3r1">.true.</config_AM_conservationCheck_write_on_startup>
-<config_AM_conservationCheck_write_on_startup ocn_grid="FRISwISC02to60E3r1">.true.</config_AM_conservationCheck_write_on_startup>
-<config_AM_conservationCheck_write_on_startup ocn_grid="FRISwISC01to60E3r1">.true.</config_AM_conservationCheck_write_on_startup>
-<config_AM_conservationCheck_write_on_startup ocn_grid="RRSwISC6to18E3r5">.true.</config_AM_conservationCheck_write_on_startup>


I agree that we want all these changes based on my comment #6642 (comment)

components/mpas-ocean/cime_config/buildnml

xylar · 2024-09-25T11:17:30Z

@mark-petersen, I've got a version of your branch with my suggested modifications here:
https://github.com/xylar/E3SM/commits/mark-petersen/mpas/correct-conservation-check/
It includes a new stealth test. I have included MPAS-Ocean output files in that test by using sed to modify the env_archive.xml file. Not sure that's kosher but I couldn't find a better way.

The test passes and results can be seen here:

/lcrc/group/e3sm/ac.xasay-davis/scratch/chrys/ERS_D.T62_oQU240.GMPAS-IAF.chrysalis_intel.mpaso-conservation_check.20240925_060737_57gegd

mark-petersen · 2024-09-25T13:49:57Z

Updated using @xylar corrections based on previous comment and comment on issue page. Thanks! Retesting now...

xylar · 2024-09-25T13:54:29Z

...onents/mpas-ocean/cime_config/testdefs/testmods_dirs/mpaso/conservation_check/shell_commands

+# include mpas-ocean outputs in testing
+sed -i 's#compclass="ocn" exclude_testing="true"#compclass="ocn" exclude_testing="false"#g' env_archive.xml


@jonbob and @rljacob, let me know what you think if this approach. Happy to hear of other suggestions for how to not exclude MPAS-Ocean files from this particular test.

Since its an xml file, can xmlchange do that? If not, sed is fine.

I couldn't figure out how. I think xmlchange can only be used to modify specific fields that have an id and value, and this is not one of those.

I slacked Jim to ask if he knows...

mark-petersen · 2024-09-25T13:56:36Z

Updated the description.

cime_config/tests.py

mark-petersen · 2024-09-25T17:15:58Z

I can now confirm the expected behavior from this PR.

I ran QU240 with config_am_conservationcheck_enable = .true. for all cases, and everything standard from the repo for four cases here /lcrc/group/e3sm/ac.mpetersen/scratch/chrys/

master, 2 months 240924i.ConsCheckMasterFull.T62_oQU240.GMPAS-IAF.chr/
master 1mo+restart+1 mo 240924j.ConsCheckMasterRestart.T62_oQU240.GMPAS-IAF.chr/
pr 2 months 240924k.ConsCheckPrFull.T62_oQU240.GMPAS-IAF.chr/
pr 1mo+restart+1 mo 240924l.ConsCheckPrRestart.T62_oQU240.GMPAS-IAF.chr/

writing restarts every month. For reference, I used

./create_newcase -case $CASE_ROOT/$E3SM_CASE   -compiler gnu   -mach chrysalis   -project e3sm \
   -compset GMPAS-IAF -res T62_oQU240 --walltime 00:30:00

In case 2, the first entry differs from case 1 by being either zero or differing:

$ ncdump -v accumulatedRainFlux  *mpaso.hist.am.conservationCheck*01-02-01.nc |tail
 accumulatedRainFlux = 12140753948.2218, 12114183191.7286, 12117918123.2804,  (case 1)
 accumulatedRainFlux = 12129038639.0062, 12114183191.7286, 12117918123.2804, (case 2)
...

$ ncdump -v accumulatedRiverRunoffTemperatureFlux *mpaso.hist.am.conservationCheck*01-02-01.nc | tail
 accumulatedRiverRunoffTemperatureFlux = 18589454.7081383, 18605091.7352952, (case 1)
 accumulatedRiverRunoffTemperatureFlux = 0, 18605091.7352952, (case 2)

Running from this PR, case 3 and case 4 have identical output, identical to case 1 above.

xylar

I'm approving based on @mark-petersen's testing and my own.

jonbob · 2024-09-25T18:50:05Z

I've been testing with the new testdef and don't believe it's working correctly -- first, it doesn't seem to change "exclude_testing" for mpaso in env_archive.xml. However, even if I change that by hand, it still does not run cprnc on the conservation am file -- it might take some work to get testing to recognize that file

xylar · 2024-09-25T19:21:13Z

@jonbob, that's odd. I think it changed the file for me. In:

/lcrc/group/e3sm/ac.xylar/scratch/chrys/ERS_D.T62_oQU240.GMPAS-IAF.chrysalis_intel.mpaso-conservation_check.20240925_060737_57gegd

I'm seeing:

    <comp_archive_spec compname="mpaso" compclass="ocn" exclude_testing="false">
      <rest_file_extension>rst</rest_file_extension>
...

as expected.

As for the latter point. I wasn't sure how to find out what cprnc is checking. I guess we'll need @jgfouca's help to figure out another way.

jonbob · 2024-09-25T19:27:06Z

@xylar -- that's very odd... Anyway, Jim didn't see a way to do it with xmlchange either.

xylar · 2024-09-25T19:34:00Z

@jonbob, I'll try it again. In the meantime, can you clarify what you are doing to see what files cprnc is looking at?

xylar · 2024-09-25T19:40:07Z

I reset to the current state of this branch and I'm also seeing that the sed changes aren't happening -- so odd!

xylar · 2024-09-25T19:51:04Z

My fault. I accidentally had e3sm-unified loaded when I launched the test. It must have a better sed that supports -i but the default sed on Chrysalis doesn't (!!!).

I'll have to do this more manually.

jonbob · 2024-09-25T20:01:16Z

@xylar -- there are files that get made for tests -- TestStatus and TestStatus.log. If I grep TestStatus.log for cprnc, I only see that being run on the cpl.hi files

xylar · 2024-09-25T20:18:18Z

Thanks @jonbob. I'll try to debug some more tomorrow.

xylar · 2024-09-25T20:24:34Z

On second thought, I'm beginning to feel like getting an ERS test to work with MPAS-Ocean history output is too big a project to be in the scope of this PR.

@jonbob and @rljacob, I think the only plausible fallback is to take out the stealth test. What about you?

jonbob · 2024-09-25T21:05:45Z

@xylar -- I was going to recommend the same thing. I think it's more important to fix the functionality, and we can add a test once we get it working

rljacob · 2024-09-25T21:38:52Z

Don't try to do the sed magic and force the comparison but leave on the test that enables the conservation analysis members.

xylar · 2024-09-26T06:06:31Z

@rljacob, okay, good suggestion. I have noted in the README for the test that it does not currently include MPAS-Ocean history files. I have given a manual change that folks could make to cime_config/config_archive.xml if they want to include these files.

jonbob · 2024-09-26T17:59:30Z

components/mpas-ocean/cime_config/testdefs/testmods_dirs/mpaso/conservation_check/README

+However, it shoudl be noted that MPAS-Ocean history files are not currently
+included in E3SM testing so non-BFB results will not be detected unless one
+manually changes to 'compname="mpaso" exclude_testing="false"' in the file
+cime_config/config_archive.xml.


one quick comment -- even changing exclude_testing won't check any of the mpaso history files. Plus there's a minor misspelling: "shoudl"

@jonbob, okay, I understood that it would but I must be mistaken. I have removed this text and fixed the typo.

@xylar -- it should, but in practice none of the history files matches the file check in env_archive.xml. I did play around with it, and it works if we modify that file with this:

<comp_archive_spec compclass="ocn" compname="mpaso"> <rest_file_extension>rst</rest_file_extension> <rest_file_extension>rst.am.timeSeriesStatsMonthly</rest_file_extension> <hist_file_extension>hist.am.conservationCheck\..*\.nc$</hist_file_extension> <rest_history_varname>unset</rest_history_varname>

where I added the conservationCheck line -- or actually replaced a line that had no effect. Now it actually does use the conservationCheck am for the test: this from TestStatus.log:

ERS_Ld5_D.T62_oQU240.GMPAS-IAF.chrysalis_intel.mpaso-conservation_check.20240925_131015_n3zwao.mpaso.hist.am.conservationCheck.0001-01-01.nc.base matched ERS_Ld5_D.T62_oQU240.GMPAS-IAF.chrysalis_intel.mpaso-conservation_check.20240925_131015_n3zwao.mpaso.hist.am.conservationCheck.0001-01-01.nc.rest .... tail -n20 /lcrc/group/e3sm/ac.jwolfe/scratch/chrys/ERS_Ld5_D.T62_oQU240.GMPAS-IAF.chrysalis_intel.mpaso-conservation_check.20240925_131015_n3zwao/run/ERS_Ld5_D.T62_oQU240.GMPAS-IAF.chrysalis_intel.mpaso-conservation_check.20240925_131015_n3zwao.mpaso.hist.am.conservationCheck.0001-01-01.nc.base.cprnc.out 1 0.000000000000000E+00 0.000000000000000E+00 1 0.000000000000000E+00 0.000000000000000E+00 1 ( 1) ( 1) avg abs field values: 0.000000000000000E+00 0.000000000000000E+00 ************************************************************************************************************************************ SUMMARY of cprnc: A total number of 290 fields were compared of which 0 had non-zero differences and 0 had differences in fill patterns and 0 had different dimension sizes and 0 had different data types A total number of 5 fields could not be analyzed A total number of 0 time-varying fields on file 1 were not found on file 2. A total number of 0 time-constant fields on file 1 were not found on file 2. A total number of 0 time-varying fields on file 2 were not found on file 1. A total number of 0 time-constant fields on file 2 were not found on file 1. diff_test: the two files seem to be IDENTICAL

Okay, clearly a bigger can of worms that remains beyond the scope of this PR. Thanks for being more thorough than I was and for keeping things honest.

jonbob · 2024-09-26T20:08:16Z

@xylar -- I tested this in the shell_commands and it does give us what we want:

sed -i 's/\(compname="mpaso" exclude_testing="true"\)/compname="mpaso"/' env_archive.xml
sed -i '/\(compname="mpaso"\)/,/<hist_file_extension>/ s/<hist_file_extension>hist/<hist_file_extension>hist.am.conservationCheck\\..*\\.nc$/' env_archive.xml

I'm not convinced we need to make the test completely functional for this PR, but I was curious about how difficult it would be...

darincomeau

Approve the changes to the conservation check based on other's testing and visual inspection.
I'll defer to others on the added testing.

…t (PR #6643) Correct ocean conservation check settings Currently, the ocean conservation check analysis member overwrites the first entry in the file with a zero after restarts for some variables. This PR corrects this behavior so that the first day's entry in a monthly conservation check file is identical between continuous runs and a run with a restart break. Adds a new mpaso testdef and corresponding stealth test. Fixes #6642 [NML] for some mpaso resolutions [BFB]

jonbob · 2024-09-26T22:32:19Z

Passes:

ERP_Ld3.ne30pg2_r05_IcoswISC30E3r5.WCYCL1850.chrysalis_intel.allactive-pioroot1 (with expected NML DIFF)
ERS_Ld5.T62_oQU120.CMPASO-NYF.chrysalis_intel

merged to next

jonbob · 2024-09-27T15:27:12Z

merged to master and expected NML DIFFs blessed

This merge updates the E3SM-Project submodule from [727ad81](https://github.com/E3SM-Project/E3SM/tree/727ad81) to [1442143](https://github.com/E3SM-Project/E3SM/tree/1442143). This update includes the following MPAS-Ocean and MPAS-Frameworks PRs (check mark indicates bit-for-bit with previous PR in the list): - [ ] (ocn) E3SM-Project/E3SM#6509 - [ ] (ocn) E3SM-Project/E3SM#6508 - [ ] (fwk) E3SM-Project/E3SM#6575 - [ ] (ocn) E3SM-Project/E3SM#6590 - [ ] (fwk) E3SM-Project/E3SM#6643 - [ ] (ocn) E3SM-Project/E3SM#6656 - [ ] (ocn) E3SM-Project/E3SM#6672 - [ ] (ocn) E3SM-Project/E3SM#6659 - [ ] (ocn) E3SM-Project/E3SM#6497 - [ ] (ocn) E3SM-Project/E3SM#6485 - [ ] (ocn) E3SM-Project/E3SM#6566

mark-petersen added mpas-ocean mpas-seaice bug fix PR labels Sep 24, 2024

mark-petersen requested review from darincomeau and xylar September 24, 2024 16:29

mark-petersen assigned jonbob Sep 24, 2024

xylar reviewed Sep 24, 2024

View reviewed changes

xylar changed the title ~~Correct ocean and sea ice conservation check settings~~ Correct ocean conservation check settings Sep 25, 2024

xylar removed the mpas-seaice label Sep 25, 2024

xylar reviewed Sep 25, 2024

View reviewed changes

components/mpas-ocean/cime_config/buildnml Outdated Show resolved Hide resolved

mark-petersen and others added 2 commits September 25, 2024 04:29

Correct ocean conservation check settings

a2fb3e7

Change conservationCheckOutput to append clobber mode

beca87f

Add a stealth ERS test for conservation check AM

ff5aa6f

mark-petersen force-pushed the mark-petersen/mpas/correct-conservation-check branch from 981303a to ff5aa6f Compare September 25, 2024 13:47

xylar reviewed Sep 25, 2024

View reviewed changes

jonbob reviewed Sep 25, 2024

View reviewed changes

cime_config/tests.py Show resolved Hide resolved

jonbob added the NML label Sep 25, 2024

xylar approved these changes Sep 25, 2024

View reviewed changes

Fix sed command

448f4f1

Update conservation_check test to remove sed command

2982d4a

jonbob reviewed Sep 26, 2024

View reviewed changes

Fix readme for the new test

87182bb

darincomeau approved these changes Sep 26, 2024

View reviewed changes

rljacob added this to the v3.0.1 milestone Sep 26, 2024

jonbob merged commit b12eb68 into master Sep 27, 2024
9 checks passed

jonbob deleted the mark-petersen/mpas/correct-conservation-check branch September 27, 2024 15:18

xylar mentioned this pull request Sep 28, 2024

Add new SOwISC12to30E3r3 ocean and sea-ice mesh #6631

Merged

xylar mentioned this pull request Oct 26, 2024

Update E3SM-Project submodule MPAS-Dev/compass#867

Merged

11 tasks

		<config_AM_conservationCheck_enable>false</config_AM_conservationCheck_enable>
		<config_AM_conservationCheck_enable>true</config_AM_conservationCheck_enable>

		# include mpas-ocean outputs in testing
		sed -i 's#compclass="ocn" exclude_testing="true"#compclass="ocn" exclude_testing="false"#g' env_archive.xml

Correct ocean conservation check settings #6643

Correct ocean conservation check settings #6643

Conversation

mark-petersen commented Sep 24, 2024 • edited by jonbob Loading

github-actions bot commented Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proteanplanet commented Sep 24, 2024 • edited Loading

xylar commented Sep 24, 2024 • edited Loading

rljacob commented Sep 24, 2024

xylar commented Sep 24, 2024

jonbob commented Sep 24, 2024

xylar commented Sep 24, 2024

xylar commented Sep 24, 2024 • edited Loading

mark-petersen commented Sep 24, 2024

Choose a reason for hiding this comment

xylar commented Sep 25, 2024 • edited Loading

mark-petersen commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-petersen commented Sep 25, 2024

mark-petersen commented Sep 25, 2024 • edited Loading

xylar left a comment

Choose a reason for hiding this comment

jonbob commented Sep 25, 2024

xylar commented Sep 25, 2024

jonbob commented Sep 25, 2024

xylar commented Sep 25, 2024

xylar commented Sep 25, 2024

xylar commented Sep 25, 2024

jonbob commented Sep 25, 2024

xylar commented Sep 25, 2024

xylar commented Sep 25, 2024

jonbob commented Sep 25, 2024

rljacob commented Sep 25, 2024

xylar commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonbob commented Sep 26, 2024

darincomeau left a comment

Choose a reason for hiding this comment

jonbob commented Sep 26, 2024

jonbob commented Sep 27, 2024

mark-petersen commented Sep 24, 2024 •

edited by jonbob

Loading

github-actions bot commented Sep 24, 2024 •

edited

Loading

proteanplanet commented Sep 24, 2024 •

edited

Loading

xylar commented Sep 24, 2024 •

edited

Loading

xylar commented Sep 24, 2024 •

edited

Loading

xylar commented Sep 25, 2024 •

edited

Loading

mark-petersen commented Sep 25, 2024 •

edited

Loading