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

New ocean variables for floating land ice #5464

Merged
merged 16 commits into from
Apr 5, 2023

Conversation

cbegeman
Copy link
Contributor

@cbegeman cbegeman commented Feb 19, 2023

This PR adds new variables to represent floating land ice. This is a necessary step toward coupling with an ice sheet model component (MALI).

  • landIceMask and landIceFraction (existing variables) now represent both grounded and floating land ice.
  • landIceFloatingMask and landIceFloatingFraction (new variables) represent only floating land ice.

The landIceFloating* variables are used to modulate land ice fluxes, whereas the landIce* variables are used to mask out certain fields, as before. landIce* variables also modulate top drag, as we want to include the effect of top drag on thin film regions to slow flow.

[NML]
[non-BFB]

@cbegeman
Copy link
Contributor Author

See E3SM-Ocean-Discussion#38 for further discussion

@cbegeman
Copy link
Contributor Author

Testing

Machine: chrysalis
Compiler/MPI lib: intel, openmpi

The PR test suite passes comparison with baseline for all tests except ocean_isomip_plus_2km_z-star_Ocean0 which is to be expected due to non-BFB changes in the initial masks (resulting in land ice flux differences at cells that were fully grounded in master and are now partially grounded). Note that the cached outputs cannot be used for the global case with ice shelf cavities because the landIceFloating* variables must be included in streams.

Furthermore, I ran all of the QUwISC240 tests. All but the analysis test pass, which is expected due to this commit E3SM-Ocean-Discussion@4212296.

tThreshMLD           Time index: 0, 1, 2, 3 0:  l1: 1.39241851630634e+03  l2: 3.25246576969646e+02  linf: 2.40783212720566e+02 1:  l1: 1.39241851630634e+03  l2: 3.25246576969646e+02  linf: 2.40783212720566e+02 2:  l1: 1.39241851630634e+03  l2: 3.25246576969646e+02  linf: 2.40783212720566e+02 3:  l1: 1.39241851630634e+03  l2: 3.25246576969646e+02  linf: 2.40783212720566e+02 FAIL

@xylar xylar self-requested a review February 19, 2023 23:22
@xylar xylar added mpas-ocean non-BFB PR makes roundoff changes to answers. labels Feb 19, 2023
@xylar
Copy link
Contributor

xylar commented Feb 19, 2023

@jonbob, I'm marking this as non-BFB for now. We changed how the mixed layer depth is computed in this PR and that definitely affects some analysis members. In @cbegeman's testing it doesn't affect the dynamics in a QUwISC240 standalone tests but that's surprising to me and I expect non-BFB but non-climate-changing differences for runs with ice-shelf cavities in E3SM. It may simply be that the standalone tests aren't configured quite the same way as E3SM.

@xylar
Copy link
Contributor

xylar commented Feb 19, 2023

As I have discussed with @cbegeman, this can't go in until we get new initial conditions for all wISC runs that include the new variables (just copies of existing fields). I will work on that soon.

New ICs:

  • oEC60to30v3wLI
  • ECwISC30to60E1r2
  • oQU240wLI
  • oRRS30to10v3wLI
  • SOwISC12to60E2r4
  • ECwISC30to60E2r1

@xylar
Copy link
Contributor

xylar commented Feb 20, 2023

I used commands like:

ncap2 -O \
  -s "landIceFloatingFraction=landIceFraction;landIceFloatingMask=landIceMask" \
  $src $dst

to add the new variables.

@xylar xylar requested a review from vanroekel February 20, 2023 19:24
!$omp parallel
!$omp do schedule(runtime)
do iCell = 1, nCells
dThreshMLD(iCell) = dThreshMLD(iCell) - abs(landIceDraft(iCell))*landIceMask(iCell)
dThreshMLD(iCell) = dThreshMLD(iCell) - abs(landIceDraft(iCell))
end do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vanroekel, this is the piece of code I'd like to get your input on. It's the only thing that I think will lead to non-BFB results in fields that could feed back on model dynamics.

I think I masked this with the landIceMask before without really thinking about the consequences. But the ice draft is smoothed near the ice-shelf calving front on both the iceward and seaward sides. It seems like we should remove this ice draft (the amount that the sea surface is depressed by the weight of an ice shelf) even in open ocean areas where it is nonzero (maybe 10 cells seaward of an ice shelf at most).

I just wanted to check with you because you'll have the best sense of what fields we should be testing for changes as a result of modifying dThreshMLD. Which parts of the eddy parameterizaiton are affected?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xylar thanks for the explanation on the change. I agree that if we have an added pressure on the sea surface we should account for this and so it makes sense to remove that iceMask.

At minimum this will cause some changes in the submesoscale eddy parameterization, my feeling is it will strengthen as I'd expect MLD to deepen with this change where the ice pressure wasn't being removed previously. So checking the MLEvelocity (normal and vertical) fields would be a good start.

If you have redi enabled, I'd expect very slight changes there as well. For that one you could take a look at the temperature_hmixTendency (or salinity) variable, assuming the tracer budgets are on. But again am expecting small changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vanroekel, thanks for your intuition on what will change.

I'd expect MLD to deepen with this change

I could be wrong but I expect the opposite -- I think the MLD was artificially deep because the old code didn't account for the fact that much of that depth was actually because the sea surface was suppressed by land ice.

So it may be that we would expect a weakening of those parameterizations around ice-shelf fronts. I'll take a look and see which it is.

Thanks again!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry you are right @xylar I thought this bit of code was correcting the pressure when finding the reference pressure level, but is a correction to the MLD itself. Yes MLD will definitely shallow with this change.

@xylar
Copy link
Contributor

xylar commented Feb 22, 2023

@jonbob, I'd like to do a bit-for-bit test for each of the 6 meshes with ice-shelf cavities. Is there a way to do that with the E3SM testing infrastructure (i.e. do we have short test runs for all 6 meshes)? If not, do you have an alternative suggestion for how I could do this efficiently?

@xylar
Copy link
Contributor

xylar commented Feb 22, 2023

@jonbob, following up, I'm going to see how far I can get with tests like:

./create_test --wait --walltime 1:00:00 -g -b master_20230221 --baseline-root /lcrc/group/e3sm/ac.xylar/e3sm_baselines SMS_Ln9.T62_oEC60to30v3wLI.GMPAS-DIB-IAF-ISMF.chrysalis_intel

Let me know if you suggest something different.

@jonbob
Copy link
Contributor

jonbob commented Feb 22, 2023

@xylar -- that's pretty much what I was going to suggest

@xylar
Copy link
Contributor

xylar commented Feb 22, 2023

Holy cow! It works!

@xylar
Copy link
Contributor

xylar commented Feb 22, 2023

I'll have to pick this up tomorrow but I should have my 6 baselines by then.

@xylar
Copy link
Contributor

xylar commented Feb 22, 2023

I ran 6 baseline tests using 0a642b7 as the baseline (the commit on master where this branch was created).

5 of 6 show diffs in many fields:
New ICs:

  • oEC60to30v3wLI
  • ECwISC30to60E1r2
  • oRRS30to10v3wLI
  • SOwISC12to60E2r4
  • ECwISC30to60E2r1

One test, oQU240wLI, fails for reasons I don't yet understand:

/lcrc/group/e3sm/ac.xasay-davis/scratch/chrys/SMS_Ln9.T62_oQU240wLI.GMPAS-DIB-IAF-ISMF.chrysalis_intel.C.20230222_105814_ja9g3r

Overall, my results are in the subdirectories of:

/lcrc/group/e3sm/ac.xasay-davis/scratch/chrys/

@cbegeman
Copy link
Contributor Author

cbegeman commented Feb 22, 2023

@xylar Regarding the failure,

I'm seeing the following error message:

ERROR:  Warning: Sea surface height outside of acceptable range (20m)

I might have seen this error prior to #5254. However, it's worth checking that nothing is wrong with the initial streams, given that it is failing right away.

@xylar
Copy link
Contributor

xylar commented Feb 22, 2023

@cbegeman, I think the actual issue is just that oQU240wLI was never set up to use data icebergs. So both the baseline and the comparison runs failed. I think the error about the SSH being outside of bounds is just because there's a check that should be turned off, but as you say it's unrelated to the main failure.

@xylar
Copy link
Contributor

xylar commented Feb 22, 2023

From the sea ice log file for both oQU240wLI runs:

WARNING: Error: Sub-pool berg_forcing_forcing_input not found in pool.
ERROR: Stream 'dataIcebergForcing' attempted to read non-existent file '/lcrc/group/e3sm/data/inputdata/ice/mpas-cice/oQU240wLI/'
CRITICAL ERROR: Forcing: get_initial_forcing_data: READ: -3

@xylar
Copy link
Contributor

xylar commented Feb 22, 2023

@jonbob, I think this is looking okay and it's time to set up a 10-year run to make sure the results are not climate changing. Let's presumably do a test merge with #5458 (or wait for that to go in) before we do that run. Otherwise, the MOC can't be included in the comparison.

Copy link
Contributor

@xylar xylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved based on my testing with 5 of 6 supported meshes with ice-shelf cavities, and on the assumption that a comparison of a 10-year runs does, indeed, show non-climate-changing results.

@jonbob
Copy link
Contributor

jonbob commented Feb 22, 2023

@xylar -- my plan is to get #5458 in today, and then I can start a couple of longer runs to test the impact. I was thinking to use the ECwISC30to60E2r1 configuration, but open to other ideas

@xylar
Copy link
Contributor

xylar commented Feb 23, 2023

@jonbob, yes, that's what I would use and what I've been testing ice-shelf work (like #5447) with.

@jonbob
Copy link
Contributor

jonbob commented Mar 2, 2023

Comparison of 10-year test with baseline, using ne30pg2_ECwISC30to60E2r1 and CRYO1850 configurations, are available at: 20230227.5464test.anvil. @xylar and @cbegeman, please make sure this looks OK to you -- and invite anyone else to review based on these results.

@cbegeman
Copy link
Contributor Author

cbegeman commented Mar 2, 2023

@xylar and @jonbob I think this looks reasonable. It made a larger difference for Amery than I would have expected, but I don't think anything is wrong with the implementation. I'm curious to see what @xylar thinks.

@xylar
Copy link
Contributor

xylar commented Mar 4, 2023

Thanks @jonbob! This analysis is really helpful.

@cbegeman, I agree that the changes around Amery are surprisingly large. But based on the spatial patterns of the salinity and temperature changes (.e.g https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.jwolfe/20230227.5464test.anvil/html/ocean/temperatureSOSE_antarctic_20230227.5464test.anvil_depth_top_JFM_years0001-0010.png and https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.jwolfe/20230227.5464test.anvil/html/ocean/salinitySOSE_antarctic_20230227.5464test.anvil_depth_top_JFM_years0001-0010.png) I would say this is ensemble variability rather than a direct result of the change in mixed layer depth.

These plots suggest that MLD changes themselves are not confined to the regions just seaward of calving fronts where the code change would be expected to have an effect:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.jwolfe/20230227.5464test.anvil/html/ocean/mixedLayerDepthSOSE_antarctic_20230227.5464test.anvil_JFM_years0001-0010.png
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.jwolfe/20230227.5464test.anvil/html/ocean/mixedLayerDepthSOSE_antarctic_20230227.5464test.anvil_JAS_years0001-0010.png
Instead, the changes here, too, look more like ensemble variability.

I'm not seeing anything here that has me worried. I think this PR is not climate changing, as we hoped.

@xylar
Copy link
Contributor

xylar commented Mar 4, 2023

@vanroekel, just waiting on your approval based on my questions about the MLD changes. It looked like you were okay with them.

Copy link
Contributor

@vanroekel vanroekel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I agree changes look non-climate changing. Happy to approve based on visual inspection of changes and testing shown within this PR.

@jonbob
Copy link
Contributor

jonbob commented Mar 16, 2023

@cbegeman -- I went to test this before merging and now getting an error from mpaso:

CRITICAL ERROR: landIceFloatingMask contains the default value which likely indicates that this field is missing in the initial condition file (e.g. because it is meant for an older E3SM version).

But the tests all use non-wISC grids -- my longer run was with ECwISC30to60E2r1.

@jonbob jonbob added the NML label Mar 30, 2023
@cbegeman
Copy link
Contributor Author

@jonbob I think that's exactly what we need. I believe I have implemented your suggestions in 01e97b8, but have not done any testing.

@xylar
Copy link
Contributor

xylar commented Apr 3, 2023

@cbegeman, the conflict is related to my changes in #5553. You'll presumably need to rebase to fix the conflicts. It looks like it's just going to be a question of indenting the changes you've made so they're inside the new if statement.

@cbegeman cbegeman force-pushed the ocn/new-floating-land-ice-vars branch from 01e97b8 to aa144a6 Compare April 3, 2023 15:57
@cbegeman
Copy link
Contributor Author

cbegeman commented Apr 3, 2023

@jonbob, @xylar Now rebased and ready for further testing

jonbob added a commit that referenced this pull request Apr 3, 2023
…5464)

New ocean variables for floating land ice

This PR adds new variables to represent floating land ice. This is a
necessary step toward coupling with an ice sheet model component (MALI).
* landIceMask and landIceFraction (existing variables) now represent
  both grounded and floating land ice.
* landIceFloatingMask and landIceFloatingFraction (new variables)
  represent only floating land ice.
The landIceFloating* variables are used to modulate land ice fluxes,
whereas the landIce* variables are used to mask out certain fields, as
before. landIce* variables also modulate top drag, as we want to include
the effect of top drag on thin film regions to slow flow.

[NML]
[non-BFB]
@jonbob
Copy link
Contributor

jonbob commented Apr 3, 2023

passes:

  • SMS_D_Ld1.ne30pg2_EC30to60E2r2.WCYCL1850.chrysalis_intel.allactive-wcprod
  • SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel
  • ERS.ne11_oQU240.WCYCL1850NS.chrysalis_intel

with expected NML DIFFs and BFB for grids without ice shelf cavities

merged to next

@jonbob jonbob merged commit c292bec into E3SM-Project:master Apr 5, 2023
@jonbob
Copy link
Contributor

jonbob commented Apr 5, 2023

merged to master and expected NML DIFFs blessed

@rljacob
Copy link
Member

rljacob commented Nov 27, 2023

Was this actually non-BFB for any of our tested cases?

@jonbob
Copy link
Contributor

jonbob commented Nov 27, 2023

@rljacob -- I don't think so, after re-reading the discussion above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mpas-ocean NML non-BFB PR makes roundoff changes to answers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants