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

Fixed error in ice thermodynamic conservation #5146

Conversation

akturner
Copy link
Contributor

Non-accounting of enthalpy removal for thin snow/ice in enthalpy adjustment can result in a non-conservation error in prescribed ice mode with short time steps. This appears to occur when small amounts of snow accumulate after removal of snow layer.

Fixes #5124

[Non-BFB]

Non-accounting of enthalpy removal for thin snow/ice in enthalpy adjustment can result in a non-conservation error in prescribed ice mode with short timesteps. This appears to occur when small amounts of snow accumulate after removal of sno layer.
@akturner
Copy link
Contributor Author

@mkstratos : Could you check this fixes the issues you've had recently with the prescribed ice mode in MPAS-Seaice?

@akturner akturner requested a review from mkstratos August 19, 2022 15:57
@mkstratos
Copy link
Contributor

Fantastic! This has solved the issue we'd been experiencing with the F2010 compset run at dtime=1.

@jonbob jonbob self-assigned this Aug 22, 2022
@jonbob jonbob added the non-BFB PR makes roundoff changes to answers. label Aug 22, 2022
@rljacob
Copy link
Member

rljacob commented Aug 22, 2022

@jonbob is this non-BFB for the v2 low res config?

@jonbob
Copy link
Contributor

jonbob commented Aug 22, 2022

@rljacob - I think it may be, since it changes convergence criteria in specific situations. I was going to touch base with you about it, once I ran some of the nightly tests for comparison. How would you prefer to handle this?

@rljacob
Copy link
Member

rljacob commented Aug 22, 2022

Just would like a note on the merge commit message about when its non-BFB.

@jonbob
Copy link
Contributor

jonbob commented Aug 22, 2022

OK, I'll make sure

@jonbob
Copy link
Contributor

jonbob commented Aug 30, 2022

I ran e3sm_developer on a test merge and everything passed except:

  • ERS.ne30pg2_r05_EC30to60E2r2.GPMPAS-JRA.chrysalis_intel.mosart-rof_ocn_2way
  • SMS_P12x2.ne4_oQU240.WCYCL1850NS.chrysalis_intel.allactive-mach_mods
  • SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.chrysalis_intel

so pretty much every test with mpas-seaice in its non-prescribed ice mode. @akturner, does this match your expectations?

@akturner
Copy link
Contributor Author

akturner commented Sep 1, 2022

@jonbob: The change is non-BFB and it would only take the particular condition to be called once for the run to be non-BFB, so I'm not surprised the BFB tests dont pass. Is there a tool to compare climatologies available?

@jonbob
Copy link
Contributor

jonbob commented Sep 1, 2022

@akturner -- there is no such tool right now, other than comparing longer runs by hand. The deep dive Mark Taylor led came up with some requirements for the next phase but they are not yet supported, so it may mean we should do at least some 10-year comparison runs just to make sure this all looks like round-off level changes.

@jonbob
Copy link
Contributor

jonbob commented Sep 8, 2022

Started two 10-year LR tests on anvil to check the impact of this non-BFB PR

@eclare108213
Copy link
Contributor

@jonbob would it be possible to output ice volume (or thickness) daily for 5 years? Then @darincomeau might be able to run a QC test on the output (maybe after this PR is merged) as part of the QC development effort?

@jonbob
Copy link
Contributor

jonbob commented Sep 8, 2022

@eclare108213 -- hmmm, these runs are already started. I am happy to make a five-year test with daily seaice output whenever you think the codebase in the repo is ready. It might make more sense to do that separately instead of leveraging these tests, so please just let me know

@jonbob
Copy link
Contributor

jonbob commented Sep 8, 2022

@akturner - the test run with this PR failed after 17 days with seaice errors. If you want to look at the output, it's on lcrc at:

/lcrc/group/acme/ac.jwolfe/scratch/anvil/20220908.PR5146.anvil/run

@darincomeau
Copy link
Member

@eclare108213 @jonbob I'm happy to run my own tests off this branch (or once its merged) for QC purposes. I'd stick with D-cases for QC purposes, and I don't imagine those would be run as part of testing here. (BTW I just make changes to the output stream to turn it on, and add iceVolumeCell as daily output for QC testing).

@rljacob
Copy link
Member

rljacob commented Sep 15, 2022

telecon notes: need to look at fail.

@rljacob rljacob added bug fix PR and removed bug labels Sep 15, 2022
@akturner
Copy link
Contributor Author

akturner commented Sep 20, 2022

@eclare108213, @njeffery : So this change should not be applied as is to adjust_enthalpy where the tracer isn't enthalpy. smliq and smice have units of freshn - should freshn be applied here instead of fhocn? Should fhocn, fsaltn, or freshn be applied to rsnw?

@njeffery
Copy link
Contributor

@akturner : smice, smliq and rsnw are not involved in ocean fluxes or conservation of fresh water, salt or heat. You could make fhocn optional, but it might be simpler to correct ocean fluxes for small snow thicknesses before the adjust_enthalpy subroutine and keep that routine for just redistributing tracers.

Adjust_enthalpy adjusts more than just entahlpy so it wasn't appropriate to uodate fhocnn for all calls. freshn and fsaltn are now used for some calls.
fhocnn is now an optional argument to adjust_enthalpy and isnt used by non-conserved quantities
@eclare108213
Copy link
Contributor

The correction to fhocn does not belong in subroutine adjust_enthalpy. That routine just remaps quantities vertically (and ought to be renamed -- it used to be only used for enthalpy, now more generally). I recommend doing the correction just before the call to adjust_enthalpy in ice_snow.F90. Then most (maybe all) of your other changes to the code are unnecessary.

@jonbob
Copy link
Contributor

jonbob commented Sep 23, 2022

I ran a ten-year test after commit 0559859 - the maps-analysis comparison with a baseline can be seen at 20220921.PR5146.anvil. I didn't see anything that makes me think this PR changes anything beyond roundoff, but @akturner please take a look. For what it's worth, I think the commit 6c95bcd has a problem, but I did not include it in my test

@akturner akturner closed this Sep 26, 2022
@akturner akturner deleted the akturner/mpas-seaice/prescribed-ice-energy-error branch September 26, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MPAS-SI crashes on small timesteps
7 participants