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 isunnguata sermia mesh generation test case #602

Merged

Conversation

trhille
Copy link
Collaborator

@trhille trhille commented Apr 14, 2023

Add a new issunguata_sermia test case with a mesh_gen test group.

Checklist

  • User's Guide has been updated
  • Developer'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

@trhille trhille added enhancement New feature or request in progress This PR is not ready for review or merging labels Apr 14, 2023
@trhille trhille changed the title Landice/add iss sermia mesh gen Add issunguata sermia mesh generation test case Apr 14, 2023
@xylar xylar added the land ice label Apr 17, 2023
@trhille trhille force-pushed the landice/add_iss_sermia_mesh_gen branch from 1bddfd9 to 728cc4a Compare April 18, 2023 16:44
@trhille
Copy link
Collaborator Author

trhille commented Apr 18, 2023

force-push from 1bddfd9 to 728cc4a was a rebase onto compass/main to incorporate changes after merge of #590.

@trhille
Copy link
Collaborator Author

trhille commented Jul 3, 2023

Testing

Running mesh_gen created the mesh without issues. It's closer to 0.8–8 km than 1–10km, but at only 3900 cells this doesn't seem like a major issue.

image

@trhille trhille requested a review from matthewhoffman July 3, 2023 18:05
@trhille trhille removed the in progress This PR is not ready for review or merging label Aug 29, 2023
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.

This looks pretty standard, so I don't have any concerns with the implementation.

Two observations:

I'm happy to entertain your opinion about to what extent either of these items should be corrected.

@trhille
Copy link
Collaborator Author

trhille commented Sep 20, 2023

Thanks @matthewhoffman. I'm happy to change the name of the test case. Good catch on the velocity-cell spacing mismatch. I believe this stems from the 8km dataset that is used to set cell spacing, while the 1km dataset is used to populate the velocity field after the mesh has been created and culled. I notice that this also leads to an issue with ice thickness abutting the mesh boundary, so I'll fix that.

@trhille trhille force-pushed the landice/add_iss_sermia_mesh_gen branch from 5a7390c to fd1d453 Compare September 22, 2023 16:47
@trhille
Copy link
Collaborator Author

trhille commented Sep 22, 2023

Force-push was a rebase onto origin/main in order to get new python cell culler.
Changing from the 8km to 2 km data set to define cell width results in much better cell spacing:
image

Note: the gridSpacing variable is now identical to the cellQuality variable. This seems like a bug introduced elsewhere in COMPASS.

@xylar
Copy link
Collaborator

xylar commented Sep 22, 2023

Note: the gridSpacing variable is now identical to the cellQuality variable. This seems like a bug introduced elsewhere in COMPASS.

That would probably be in MPAS-Tools in the cell culler? If you want to follow up, that would be appreciated. But I also don't think we use the cell quality for anything...

@matthewhoffman matthewhoffman changed the title Add issunguata sermia mesh generation test case Add isunnguata sermia mesh generation test case Sep 28, 2023
@matthewhoffman
Copy link
Member

@trhille , this looks ready to merge with exception of correcting the glacier spelling (which is a lot of changes in file names, variables, docs, etc.). Do you want to do that or leave it as is? My preference would be to fix it, and I'm happy to help do that.

@trhille
Copy link
Collaborator Author

trhille commented Jan 30, 2024

@matthewhoffman, thanks for the reminder. I'll fix that.

Add issunguata sermia test group with mesh generation test case.
Use 2km instead of 8km data set to calculate cell width. The 8km
data set cause poor cell spacing near the terminus, and an overlap
of the ice thickness with the mesh boundary.
Fix spelling error by changing name from Issunguata Sermia to
Isunnguata Sermia.
Use updated greenland files from LCRC server that have positive
basal heat flux.
@trhille trhille force-pushed the landice/add_iss_sermia_mesh_gen branch from 198d794 to d6e62ff Compare January 30, 2024 18:03
Rename png file for Isunnguata Sermia to use correct spelling. Also
fix short underlines in docs.
@trhille trhille force-pushed the landice/add_iss_sermia_mesh_gen branch from 3633159 to a261133 Compare January 30, 2024 20:25
@trhille
Copy link
Collaborator Author

trhille commented Jan 30, 2024

@matthewhoffman, I went through and fixed that spelling error, and also updated the gridded data set being used to avoid the negative basal heat flux error. This is ready for you whenever.

@matthewhoffman matthewhoffman self-requested a review February 5, 2024 20:51
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.

Thanks for the updates, @trhille ! I confirmed I can reproduce the same result.

image

@matthewhoffman matthewhoffman merged commit 3d88259 into MPAS-Dev:main Feb 5, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request land ice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants