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

Extend NASA GrIS regions into ocean #210

Conversation

alexolinhager
Copy link
Contributor

This PR introduces manually modified versions of the NASA GrIS in geometric_data/landice/region (eastCentralGreenland1, etc.), so that each region now extends into the ocean to the continental shelf. The boundaries of some regions were also modified so one region flows into each fjord. For now, the PR adds the newly modified regions without replacing the originals in case existing analyses are dependent on these regions.

@alexolinhager alexolinhager force-pushed the alexolinhager/geometric_features/add_oceanExtended_GrIS_regions branch 4 times, most recently from 43b1270 to d483e79 Compare October 29, 2024 23:36
@alexolinhager alexolinhager force-pushed the alexolinhager/geometric_features/add_oceanExtended_GrIS_regions branch from d483e79 to 4b1670a Compare October 31, 2024 19:09
@alexolinhager alexolinhager force-pushed the alexolinhager/geometric_features/add_oceanExtended_GrIS_regions branch from 4b1670a to 6c3b455 Compare October 31, 2024 20:21
Copy link
Collaborator

@cbegeman cbegeman left a comment

Choose a reason for hiding this comment

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

@alexolinhager I think this looks good. My only suggestion would be to change the names of the greenland regions aggregators so that the difference between the ocean and land ice ones are clear since both include land ice and continental shelf regions. Maybe "ISMIP6 Greenland" and "NASA Greenland"? https://github.com/MPAS-Dev/geometric_features/blob/main/geometric_features/aggregation/ocean/greenland_regions.py

Changes the names of the ISMIP6 and NASA greenland regions to make it easier
to distinguish between the two
@alexolinhager
Copy link
Contributor Author

Thanks @cbegeman, that's a good suggestion. I just pushed a commit with a new naming convention

Copy link
Collaborator

@cbegeman cbegeman 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 making those naming changes/ I think that will be more clear to users. Great work!

@xylar
Copy link
Collaborator

xylar commented Nov 7, 2024

@alexolinhager, I see that your features aren't using the geojson formatting that is standard here, you're using a more compact format that isn't as easy to read. Could you follow the second procedure here to merge them all into a single feature collection and then split them back out into individual features to clean things up?
https://mpas-dev.github.io/geometric_features/main/adding_features.html

@xylar
Copy link
Collaborator

xylar commented Nov 7, 2024

The important bit is:

# split the feature collection back into individual features within
# ./geometric_data to clean things up
gf.split(fc)

It looks like you have the tags in the database and all that.

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 , this looks good. I just have a few minor suggestions.

I loaded all the new regions (green) into QGIS and overlaid the GIS outline (pink) and one of the ISMIP6 ocean regions (red) for comparison. The background is BedMachine bed topography colored between -2000 and 0 m. Everything looks as expected! This will be nice to have available.

image

docs/aggregation.rst Outdated Show resolved Hide resolved
docs/aggregation.rst Show resolved Hide resolved
geometric_features/aggregation/__init__.py Outdated Show resolved Hide resolved
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 making these adjustments. Looks good to me!

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.

Looks good to me. I'll merge.

@xylar xylar merged commit 38808bd into MPAS-Dev:main Nov 19, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants