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

Add script to mask off frozen regions of hydro domain #562

Merged
merged 6 commits into from
Apr 10, 2024

Conversation

alexolinhager
Copy link
Contributor

Creates a new script, floodFillThawedIce.py, that deactivates frozen regions of the hydro domain by using a flood fill of thawed basal ice originating at the grounding line. Thickness of frozen ice is set to zero and a no-flow waterFluxMask is defined surrounding the frozen area.

Creates a new script, floodFillThawedIce.py, that deactivates frozen
regions of the hydro domain by using a flood fill of thawed basal ice
originating at the grounding line. Thickness of frozen ice is set to
zero and a no-flow waterFluxMask is defined surrounding the frozen
area.
@alexolinhager alexolinhager requested review from dimomatt and matthewhoffman and removed request for dimomatt April 5, 2024 20:47
@xylar
Copy link
Collaborator

xylar commented Apr 6, 2024

@alexolinhager, if you want this to be available in the mpas_tools conda package, it will also need to be added here:
https://github.com/MPAS-Dev/MPAS-Tools/blob/master/conda_package/setup.py#L70
and ideally you would add some sort of test to make sure it exists somewhere around here:
https://github.com/MPAS-Dev/MPAS-Tools/blob/master/conda_package/recipe/meta.yaml#L121

A final note that we typically use script names that are lowercase with underscores but I'll leave it up to @matthewhoffman how strict things are with the land ice scripts.

Establishes UbThresh in floodFillThawedIce.py as an input variable with
a default value of 25 m/yr
@alexolinhager
Copy link
Contributor Author

Thanks @xylar, right now we just want this to be a script we can refer to and will use occasionally, but that doesn't need to be part of the conda package. I'll change the name though to make sure it's consistent with the rest of MPAS-Tools

@xylar
Copy link
Collaborator

xylar commented Apr 8, 2024

Okay, sounds good!

Copy link
Member

@matthewhoffman matthewhoffman left a comment

Choose a reason for hiding this comment

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

@alexolinhager , thanks for opening a PR for this script. It will be good to have this saved for future use. I have requested some changes. A few will change the result, so take a careful look at things after making the adjustments and make sure those suggestions make sense.

landice/mesh_tools_li/floodFillThawedIce.py Outdated Show resolved Hide resolved
landice/mesh_tools_li/floodFillThawedIce.py Outdated Show resolved Hide resolved
landice/mesh_tools_li/floodFillThawedIce.py Outdated Show resolved Hide resolved
landice/mesh_tools_li/floodFillThawedIce.py Outdated Show resolved Hide resolved
landice/mesh_tools_li/floodFillThawedIce.py Outdated Show resolved Hide resolved
landice/mesh_tools_li/floodFillThawedIce.py Outdated Show resolved Hide resolved
landice/mesh_tools_li/floodFillThawedIce.py Outdated Show resolved Hide resolved
landice/mesh_tools_li/floodFillThawedIce.py Outdated Show resolved Hide resolved
landice/mesh_tools_li/floodFillThawedIce.py Outdated Show resolved Hide resolved
landice/mesh_tools_li/floodFillThawedIce.py Outdated Show resolved Hide resolved
Makes minor edits to the PR in accordance with the first round of review
that improve performance and clarity.
Copy link
Member

@matthewhoffman matthewhoffman left a comment

Choose a reason for hiding this comment

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

@alexolinhager , thanks for the changes. I re-reviewed and just noticed one minor typo I made a suggestion for. Once you fix that and rename the script I can go ahead and merge this.

One other thing - can you comment on what impact the changes to the logic had? (changing growMask to use an or of the two conditions; considering thawed regions only where BMB is negative rather than nonzero)

landice/mesh_tools_li/floodFillThawedIce.py Outdated Show resolved Hide resolved
@alexolinhager
Copy link
Contributor Author

@matthewhoffman I made a few more changes to the script, mostly just minor debugging to allow the script to work with different input files that may have various variables in it that we want to get rid of. I did change back the calculation of basalMeltInput to abs(groundedBasalMassBal) though. For some reason I don't understand the previous implementation was creating all zeros throughout the domain. It seems like groundedBasalMassBal should almost always be negative, so it doesn't seem like this would have too much of an detrimental effect.

The change from and to or in the growMask definition was certainly necessary. Judging by the figure below (thawed areas are outlined in black), using an and requirement would have limited the active hydro domain to the small regions where both thawed ice and basalSlidingSpeed > 25 m/yr overlap, which would drastically cut down on the active hydro domain.
Screenshot 2024-03-22 at 2 47 30 PM

Based on the plots below though, I am confident the current script is doing what we want it to do. Non-zero thickness only exists in areas where basalMeltInput > 0, basalSlidingSpeed > 25 m/yr, and that are connected to the grounding line. Using the additional basalSlidingSpeed criteria allows us to capture the Whillans Ice Stream while having a minimal effect on the rest of the domain
Screenshot 2024-04-09 at 5 46 33 PM
Screenshot 2024-04-09 at 5 46 48 PM
Screenshot 2024-04-09 at 5 47 11 PM

alexolinhager and others added 2 commits April 10, 2024 14:34
Fix typo in description

Co-authored-by: Matt Hoffman <wyeast@gmail.com>
@alexolinhager alexolinhager force-pushed the floodFillThawedIce.py branch 2 times, most recently from 7a91efa to 74de4ea Compare April 10, 2024 21:50
Fixes a few minor bugs in the script
Copy link
Member

@matthewhoffman matthewhoffman left a comment

Choose a reason for hiding this comment

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

@alexolinhager, looks good, thanks for making the changes

@matthewhoffman matthewhoffman merged commit 12df2d5 into MPAS-Dev:master Apr 10, 2024
7 checks passed
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.

3 participants