Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Modify Western Weddell definition and add polygon functions #195

Merged
merged 12 commits into from
Apr 10, 2023

Conversation

cbegeman
Copy link
Collaborator

@cbegeman cbegeman commented Mar 28, 2023

The Western Weddell Sea region has several drawbacks which this PR resolves:

  • It does not include the full FRIS cavity
  • It extends past the Antarctic Peninsula

In order to define the region, functions to define polygons from vectors of latitude and longitude were also added.

Before:
image
After:
image

@milenaveneziani
Copy link
Collaborator

milenaveneziani commented Mar 28, 2023

Nice modification @cbegeman.

@cbegeman
Copy link
Collaborator Author

@xylar Can you let me know if you are amenable to this change? @matthewhoffman pointed this out in the context of our FRIS analysis. I don't think we are going to rerun MPAS-Analysis for those tests but I figured if I didn't address this now I would forget. I could modify the Weddell Sea region as well, and then we would also want to shift the bounds of the Bellingshausen Sea so that we have full, non-overlapping coverage.

@cbegeman
Copy link
Collaborator Author

I have not yet tested this PR with MPAS-Analysis, I just wanted to socialize these changes first since I have never worked in this repo before.

@xylar
Copy link
Collaborator

xylar commented Mar 28, 2023

I'm good with this. The original definition was pretty crude, from http://link.springer.com/10.1007/s10236-013-0642-0. Since we don't really do direct comparisons, it isn't important to match that paper.

I agree, let's change the full Weddell and Bellingshausen, too.

@cbegeman cbegeman force-pushed the modify_weddell_region branch from ecf6e30 to 222b77d Compare March 28, 2023 21:04
cbegeman and others added 5 commits March 28, 2023 18:21
Remove the second (unused) Weddell Sea feature.
We need to exclude grounded ice from the bed or we can get a longest
contour that is mostly under the ice sheet in West Antarctica.
@xylar
Copy link
Collaborator

xylar commented Apr 2, 2023

@cbegeman, I think this is now working in the sense that it produces features based on the polygons you specified. However, I don't think the FRIS cavity is being captured properly. I think the issue is both that the western boundary of the Western Weddell Sea is too far east and a bit too far north. See:
https://github.com/MPAS-Dev/geometric_features/blob/16d02dae4370c2dd07569ba309f4f24cd83b1443/geometric_data/ocean/region/Bellingshausen_Sea/region.geojson
https://github.com/MPAS-Dev/geometric_features/blob/16d02dae4370c2dd07569ba309f4f24cd83b1443/geometric_data/ocean/region/Western_Weddell_Sea/region.geojson

@xylar
Copy link
Collaborator

xylar commented Apr 2, 2023

It also seems like the Bellignshausen and Western Weddell Seas overlap by this new set of definitions:
image

I think they would ideally have a common border that is defined by 2 (or more) identical points.

@cbegeman
Copy link
Collaborator Author

cbegeman commented Apr 2, 2023

@xylar Thanks for testing that out. This is the map that uses my suggested changes. I verified in Quantarctica that these shape files include ISC, and that the southern boundaries are roughly consistent with basins, in the case that we use these regions with GL retreat. I used the same points for the AP boundary in all cases, so we shouldn't have a problem with overlapping regions.
image

@xylar
Copy link
Collaborator

xylar commented Apr 2, 2023

@cbegeman, yep, that looks right. Please commit a fixup and rebase (if you like). I can update the geojson files after that.

@cbegeman
Copy link
Collaborator Author

cbegeman commented Apr 2, 2023

@xylar I added the commit but a rebase wasn't needed. Thanks in advance for updating the geojson files!

@cbegeman cbegeman marked this pull request as ready for review April 2, 2023 20:13
@cbegeman cbegeman requested a review from xylar April 2, 2023 20:13
@xylar
Copy link
Collaborator

xylar commented Apr 2, 2023

but a rebase wasn't needed

I guess that's a question of style. I don't personally like to keep fixup commits, I merge them with the commits they are meant to fix by rebasing. A discussion for another time...

Thanks in advance for updating the geojson files!

Great, I'll update them now.

@xylar
Copy link
Collaborator

xylar commented Apr 2, 2023

@cbegeman, I'd like to rebase to combine the 2 commits I put in to update features. It's especially desirable to not have too many versions of files like the geojsons in the history (because they're relatively large). But since it's your branch, I will only rebase if you allow me to.

@cbegeman
Copy link
Collaborator Author

cbegeman commented Apr 3, 2023

@xylar I see. I thought you were just talking about a rebase in the context of an updated main. Sure, go ahead and push the fixed-up commits. Thanks!

@xylar
Copy link
Collaborator

xylar commented Apr 3, 2023

@cbegeman, ah, one more thing occurs to me. We need to update the date for the aggregator for these regions. This is how MPAS-Analysis will know that there are new regions and that it either needs to look for new files for the given mesh or make them. I'll add this.

@xylar
Copy link
Collaborator

xylar commented Apr 3, 2023

@cbegeman, I would like to test this out in MPAS-Analysis before I merge. Just need to find the time...

@cbegeman
Copy link
Collaborator Author

cbegeman commented Apr 3, 2023

@xylar Thanks for demonstrating the process. Let me know if you want to pass the testing back to me.

@milenaveneziani
Copy link
Collaborator

Hi @cbegeman, @xylar: thanks for changing these features, I think it's a good idea.

I have one question related to the Weddell Sea. Is this how it looks like now?:

https://github.com/MPAS-Dev/geometric_features/blob/8b0c64753c719009f98027c56a35d82f7d5f6dab/geometric_data/ocean/region/Weddell_Sea/region.geojson

if so, maybe something went wrong in defining the eastern Weddell because that box should not go all that far north, but follow the boundaries in the figure that @cbegeman posted at some point above.

@xylar
Copy link
Collaborator

xylar commented Apr 4, 2023

@milenaveneziani, we can fix that. I will note that the definition of the Weddell Sea that we had previously also has that feature:
https://github.com/MPAS-Dev/geometric_features/blob/main/geometric_data/ocean/region/Weddell_Sea/region.geojson
I posted this here:
#104 (comment)
There's a fairly lengthy discussion in #104.

But feel free to suggest further alterations to the Eastern and Western Weddell polygons so that they have a continuous northern boundary.

@milenaveneziani
Copy link
Collaborator

oh wow, that was a nice discussion we had in #104: it's so good that you remember it! I can see the reasons for why I was advocating for a northern border of the Eastern Weddell to be at 55S. Here is the figure I pointed to in that discussion:
image

I leave it up to you to decide whether we want to keep the Weddell as simple merge of two boxes or whether to make the Eastern Weddell a more complicated polygon. (Also, apologies for the delay in replying to this: I was taking a mini-spring break vacation).

@milenaveneziani
Copy link
Collaborator

Here is an option, if you want to make it complex:
image

That leaves the Western Weddell unchanged and only adjusts the northwestern border of the Eastern Weddell by gradually reaching 55S from 60S.

@xylar xylar force-pushed the modify_weddell_region branch from 7226b5b to 680381a Compare April 5, 2023 15:05
@xylar
Copy link
Collaborator

xylar commented Apr 5, 2023

Hmm, there's still a bit of a discontinuity along the northern boundary of the Weddell Sea feature:
https://github.com/MPAS-Dev/geometric_features/blob/680381abbdffcdc18f21d573caf29a329db1edd7/geometric_data/ocean/region/Weddell_Sea/region.geojson
Did I make a mistake?

Comment on lines 285 to 286
lon_vector=[-100., -86., -63., -66., -60., -51., 0., 0.],
lat_vector=[-81., -75., -73., -67., -64., -62., -62., -90.],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
lon_vector=[-100., -86., -63., -66., -60., -51., 0., 0.],
lat_vector=[-81., -75., -73., -67., -64., -62., -62., -90.],
lon_vector=[-100., -86., -63., -66., -60., -51., 0., 0.],
lat_vector=[-81., -75., -73., -67., -64., -60., -56.7, -90.],

And this would need to be adjusted too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it still make sense to have a separate feature for the full Weddell? Or can we just combine the the Eastern and Western (as we do below) and then split them into shelf and deep? We had a separate definition for the full Weddell before because we wanted to have the shelf/deep definitions match the Timmermann and Helmer paper but that doesn't apply anymore with these changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I would prefer to remove the separate definition of the Weddell Sea and just split the combined Eastern and Western. I will do that as soon as we agree on the definition of the Eastern below.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Were we using the Timmermann and Helmer paper for comparison? If not, and also considering that the new regions depart from the definition in that paper, I agree that we could just keep one Weddell.

I guess I am a little confused about why Caroline's modifications still had a discontinuity, but I hope the latest changes work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Were we using the Timmermann and Helmer paper for comparison?

Yes, we were at some point but we can't do that either way at this point. We have modified all 3 Weddell regions and the Bellighshausen region compared with that paper. I don't think we're that keen to keep comparing with them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great. I can commit this change soonish.

@xylar xylar force-pushed the modify_weddell_region branch from a3fce66 to a5cff9f Compare April 6, 2023 08:11
@xylar
Copy link
Collaborator

xylar commented Apr 6, 2023

Okay, I really hope we're done once and for all here:
https://github.com/MPAS-Dev/geometric_features/blob/a5cff9fda742174c9cbba20e2300484791f265f2/geometric_data/ocean/region/Eastern_Weddell_Sea/region.geojson
https://github.com/MPAS-Dev/geometric_features/blob/a5cff9fda742174c9cbba20e2300484791f265f2/geometric_data/ocean/region/Weddell_Sea/region.geojson

These both look good to me!

I still need to test on MPAS-Analysis. I discovered that I accidentally deleted the test simulation I normally use so I need to run a new one.

@cbegeman
Copy link
Collaborator Author

cbegeman commented Apr 6, 2023

@xylar That looks good! Thanks for getting to this. I clearly didn't find the time yesterday.

@milenaveneziani
Copy link
Collaborator

Looks great to me too! Thank you both for this.

@xylar
Copy link
Collaborator

xylar commented Apr 10, 2023

Okay, I tested this in MPAS-Analysis (after having to do 4 or 5 unrelated bug fixes) and things seem good:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xasay-davis/analysis_testing/chrysalis/update_geometric_features/main_py3.11/ocean/index.html#AntarcticRegions

@xylar
Copy link
Collaborator

xylar commented Apr 10, 2023

Note the inset here:
image

@xylar
Copy link
Collaborator

xylar commented Apr 10, 2023

I will merge this, then do a release of geometric_features, then update MPAS-Analysis to use this release.

@cbegeman
Copy link
Collaborator Author

Thanks @xylar!

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants