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 QGreenland monthly median sea ice extent shape files #211

Conversation

alexolinhager
Copy link
Contributor

Adds the monthly median sea ice extent (1981–2010) shape files from the QGreenland dataset. The original line segments delineating sea ice extent have been manually connected to create one coherent shape file.

@alexolinhager alexolinhager force-pushed the alexolinhager/geometric_features/add_QGreenland_median_sea_ice_extent branch 6 times, most recently from 7ef0faa to a930331 Compare October 31, 2024 18:26
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 Can you provide any other information in the docs that would help users track down the source of this dataset? Maybe a permanent identifier if that exists?

{
"type": "Feature",
"properties": {
"name": "December Median Sea Ice Extent (1981-2010)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like the name got changed to December but the directory is still April. That shouldn't be possible if you do gf.split(fc) to split out the features into the directories in geometric_data. But I think April is probably correct and the change to December was a mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @xylar. That was just a typo copied from another file, I just pushed a fix for it

@alexolinhager
Copy link
Contributor Author

@alexolinhager Can you provide any other information in the docs that would help users track down the source of this dataset? Maybe a permanent identifier if that exists?

Good idea, I just added that to the metadata for each file

@matthewhoffman
Copy link
Member

@alexolinhager Can you provide any other information in the docs that would help users track down the source of this dataset? Maybe a permanent identifier if that exists?

@alexolinhager , can you add the reference to the documentation as well?

@matthewhoffman
Copy link
Member

@alexolinhager , when I add these regions to QGIS and project to a polar stereographic grid, I see seams at 180 degree longitude. I remember a discussion about this but I can't find it - is this expected?

image

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 putting this together. The PR looks good and I just have a few small stylistic requests, plus a question about a seam in the region when plotted as polar stereographic.

@@ -0,0 +1,33 @@
def qgreenland(gf):
Copy link
Member

Choose a reason for hiding this comment

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

@xylar , is there a coding style guideline for this repo about correspondence between function/class names and file names?

geometric_features/aggregation/__init__.py Outdated Show resolved Hide resolved
geometric_features/geometric_features.py Outdated Show resolved Hide resolved
geometric_features/geometric_features.py Outdated Show resolved Hide resolved
geometric_features/geometric_features.py Outdated Show resolved Hide resolved
geometric_features/geometric_features.py Outdated Show resolved Hide resolved
@alexolinhager
Copy link
Contributor Author

@alexolinhager , when I add these regions to QGIS and project to a polar stereographic grid, I see seams at 180 degree longitude. I remember a discussion about this but I can't find it - is this expected?

image

@matthewhoffman, Thanks for looking over these PRs. The seam is expected for these regions and comes from this script:

. Without it, geojson shape files crossing the 180 degree longitude or the poles become disjointed.

@xylar said they look as he would expect, although I think that conversation happened via Slack

@alexolinhager
Copy link
Contributor Author

@alexolinhager Can you provide any other information in the docs that would help users track down the source of this dataset? Maybe a permanent identifier if that exists?

@alexolinhager , can you add the reference to the documentation as well?

No problem, I made this change

@xylar
Copy link
Collaborator

xylar commented Nov 19, 2024

@alexolinhager, there are some conflicts here. I can fix them but the cleanest would be if you're okay with rebasing onto main and fixing them that way.

@alexolinhager
Copy link
Contributor Author

@alexolinhager, there are some conflicts here. I can fix them but the cleanest would be if you're okay with rebasing onto main and fixing them that way.

No problem, I'll work on that now

@alexolinhager alexolinhager force-pushed the alexolinhager/geometric_features/add_QGreenland_median_sea_ice_extent branch from 5fd9be5 to 7b48aca Compare November 19, 2024 18:57
@alexolinhager
Copy link
Contributor Author

@Xyler, I rebased the branch and pushed a couple other minor tweaks in accordance with Matt's review. Should be ready to merge now

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 additional changes. 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.

@xylar
Copy link
Collaborator

xylar commented Nov 19, 2024

@cbegeman, do you have time to approve or request further changes?

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 All of my suggestions were addressed. Thanks for your hard work on this!

@xylar xylar merged commit 22e0b75 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