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

Fixup ISOMIP+ land ice pressure #723

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

cbegeman
Copy link
Collaborator

@cbegeman cbegeman commented Oct 25, 2023

This PR changes the calculation of land ice pressure for ISOMIP+ thin-film test cases so that it is a function of the land ice thickness rather than land ice draft. The land ice pressure is not perfectly identical between Ocean0 and thin_film_Ocean0 test cases, mostly because in the former case we smooth the draft and in the latter we smooth ice thickness. This PR also fixes a bug wherein the thin film region was culled from the domain.

Checklist

  • User's Guide has been updated
  • API documentation in the Developer's Guide (api.rst) has any new or modified class, method and/or functions listed
  • Documentation has been built locally and changes look as expected
  • Document (in a comment titled Testing in this PR) any testing that was used to verify the changes

@cbegeman
Copy link
Collaborator Author

cbegeman commented Oct 25, 2023

Testing

Ocean0 and thin_film_Ocean0 test cases have been run on chrys with intel, openmpi. thin_film_wetting_Ocean0 has been run through initial_state step to verify time-varying forcing.

@cbegeman
Copy link
Collaborator Author

@xylar Can you let me know what you think? I ended up doing something a little different from you to make the Ocean0 and thin_film_Ocean0 land ice pressure fields more similar. Open to other ideas though.

@cbegeman cbegeman self-assigned this Oct 25, 2023
@cbegeman cbegeman added bug Something isn't working ocean labels Oct 25, 2023
compass/ocean/iceshelf.py Show resolved Hide resolved
compass/ocean/tests/isomip_plus/initial_state.py Outdated Show resolved Hide resolved
ref_density = constants['SHR_CONST_RHOSW']
landIcePressure, landIceDraft = compute_land_ice_pressure_and_draft(
ssh=ds.ssh, modify_mask=ds.ssh < 0., ref_density=ref_density)
land_ice_draft = ds.ssh
Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason I don't do this for the thin film case in Polaris is because the ocean won't want to have the SSH at the flotation height that BISICLES wants to give it. This is at least partly because BISICLES used a different reference density for the ocean (1028 instead of 1026) and partly because it has been my experience that computing the ice draft (SSH) from the land-ice pressure and the reference ocean density tends to produce the most reliable starting point for the SSH adjustment step.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if this will help with the state validation issues during SSH adjustment for the thin-film case but I think it might.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I discovered that I was missing a check in my Polaris computation of land_ice_draft:

            land_ice_draft = np.maximum(ds_topo.bedrockTopography,
                                        land_ice_draft)

We obviously don't what to allow the ice draft to go deeper than the bedrock in the thin film region.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If that's the case, then wouldn't we want to assign the ssh to the recomputed land ice draft for both thin film and non-thin film cases?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could certainly do that. It would dovetail better with the coupling strategy.

The reason I had been working with SSH/draft as the known quantity and land-ice pressure as the one to adjust was that this is more how we operate with static cavities with realistic topography (e.g. from BedMachine). Without wetting and drying, you can end up with too thin a water column if you are using land-ice pressure to determine SSH. But we could either adjust the grounding line if this happens. We can't adjust the minimum column thickness unless we also change the land-ice pressure because otherwise the pressure will just slam the column back down.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I modify the bottom depth to maintain min water column thickness here:

# Deepen the bottom depth to maintain the minimum water-column
# thickness
min_column_thickness = section.getfloat('min_column_thickness')
min_layer_thickness = section.getfloat('min_layer_thickness')
min_levels = section.getint('minimum_levels')
min_column_thickness = max(min_column_thickness,
min_levels * min_layer_thickness)
min_depth = -ds.ssh + min_column_thickness
ds['bottomDepth'] = np.maximum(ds.bottomDepth, min_depth)
print(f'Adjusted bottomDepth for '
f'{np.sum(ds.bottomDepth.values<min_depth.values)} cells '
f'to achieve minimum column thickness of {min_column_thickness}')
But I only do this for the initial state so it wouldn't guarantee for a time-varying pressure that the water column thickness would stay above the threshold without wetting and drying.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right. In my experience, the only to have things behave robustly without wetting-and-drying has been to treat the water-column thickness as the thing you know and the land-ice pressure as the thing you can adjust (in the poorly named "ssh adjustment" step). Even if you thicken the water column in the initial condition, if the land-ice pressure is too high, it will just slam right back down and try to evacuate the water column.

@cbegeman
Copy link
Collaborator Author

@xylar Does the latest commit look like what you have in mind? It results in these water column thicknesses:
Ocean0:
image
thin_film_Ocean0:
image

@xylar
Copy link
Collaborator

xylar commented Oct 26, 2023

Does the latest commit look like what you have in mind?

Yes exactly, that looks great to me! Are you happy with it thought? I definitely want to make sure we're on the same page.

@cbegeman
Copy link
Collaborator Author

@xylar I'm fine with that. I can add the same thing to the time-varying forcing. I assume we keep the floating fraction the same, to mimic a case where the GL is different in MALI from MPAS-O. Do you agree?

@xylar
Copy link
Collaborator

xylar commented Oct 26, 2023

Right, I agree. We want to treat the floating fraction as something that is prescribed or supplied by MALI, so it doesn't get updated based on whether MPAS-Ocean thinks the cell is wet or dry.

@cbegeman cbegeman marked this pull request as ready for review October 26, 2023 23:39
@cbegeman
Copy link
Collaborator Author

@xylar Following up on your question earlier: Wetting and drying is on for thin-film cases, forward and ssh-adjustment steps, so that is not the issue with ssh_adjustment failures.

@cbegeman
Copy link
Collaborator Author

cbegeman commented Nov 8, 2023

@xylar Just wanted to make sure this was still on your radar to review. No particular rush.

@xylar
Copy link
Collaborator

xylar commented Nov 8, 2023

Yes, I think this was where we were discussing going back to one strategy where we trust landIcePressure and dig when necessary to prevent drying (for non-W&D cases). Did you want to have this reviewed as it is or for me to make that change here?

@cbegeman
Copy link
Collaborator Author

cbegeman commented Nov 8, 2023

Yes, I think this was where we were discussing going back to one strategy where we trust landIcePressure and dig when necessary to prevent drying (for non-W&D cases). Did you want to have this reviewed as it is or for me to make that change here?

@xylar Right, that's worth chatting about. I was assuming we merge this PR and tackle the digging strategy in a separate PR.
If you want to tackle it in this PR, that's fine by me. It would be good to have you review at least the thin film approach here because that's what I'll use in my W&D testing in the very near future.

@xylar
Copy link
Collaborator

xylar commented Nov 8, 2023

Okay, let's just focus on the thin film here and I'll make a follow-up PR to change the strategy for the non-thin-film cases. I could do a rush review tonight but it would be better if it's on my list for tomorrow.

Copy link
Collaborator

@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.

I ran all of the 5 km test cases. I'm seeing a failure in initial_state in:

ocean/isomip_plus/planar/5km/z-star/time_varying_Ocean0

because of the missing landIceDraft variable noted below.

I'm seeing state validation failures in the performance steps of:

ocean/isomip_plus/planar/5km/z-star/thin_film_time_varying_Ocean0
ocean/isomip_plus/planar/5km/sigma/thin_film_time_varying_Ocean0
ocean/isomip_plus/planar/5km/sigma/thin_film_wetting_Ocean0
ocean/isomip_plus/planar/5km/single_layer/thin_film_drying_Ocean0
ocean/isomip_plus/planar/5km/single_layer/thin_film_time_varying_Ocean0
ocean/isomip_plus/planar/5km/single_layer/thin_film_wetting_Ocean0

This is causing the test to hang, which is especially annoying but unrelated to this PR, of course.

I'm not sure how many of these need investigation as part of this PR and which we just let be for now and investigate going forward. That's partly because there are now a lot of tests and I'm not sure I have a good handle on when they're all supposed to be working.

compass/ocean/tests/isomip_plus/initial_state.py Outdated Show resolved Hide resolved
@cbegeman cbegeman force-pushed the ocn-fix-grounded-land-ice-pressure branch from 3902ae8 to 0a21b83 Compare November 27, 2023 19:06
@cbegeman cbegeman force-pushed the ocn-fix-grounded-land-ice-pressure branch from 0a21b83 to 73e20b9 Compare January 30, 2024 23:45
@cbegeman
Copy link
Collaborator Author

cbegeman commented Feb 1, 2024

@xylar FYI: I'm finding that the run hangs at the performance step of time_varying_Ocean0. I'll ping you when I figure out why that is.

@xylar
Copy link
Collaborator

xylar commented Feb 1, 2024

@cbegeman, I think the hanging itself isn't related to this PR. It seems to happen sometimes when MPAS-Ocean fails with state validation errors but I don't understand why this leads to hanging as opposed to MPAS-Ocean returning an error.

But it's worth trying to fix the state validation issues so the hanging doesn't happen.

@cbegeman cbegeman force-pushed the ocn-fix-grounded-land-ice-pressure branch from 73e20b9 to aea2ec7 Compare July 5, 2024 15:30
@cbegeman cbegeman closed this Jul 5, 2024
@cbegeman
Copy link
Collaborator Author

cbegeman commented Jul 5, 2024

Oops! comment posted on the wrong PR

@cbegeman cbegeman reopened this Jul 5, 2024
@cbegeman cbegeman force-pushed the ocn-fix-grounded-land-ice-pressure branch from aea2ec7 to e285b25 Compare October 24, 2024 16:12
@cbegeman
Copy link
Collaborator Author

@xylar I haven't yet tested all the cases, but this seems to be working now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ocean
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants