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

Use higher resolution land-sea mask #1006

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Use higher resolution land-sea mask #1006

wants to merge 3 commits into from

Conversation

Sbozzolo
Copy link
Member

@Sbozzolo Sbozzolo commented Oct 9, 2024

As added in CliMA/ClimaArtifacts#49

30 arcseconds is already pretty high resolution

Closes #936
Closes #730
Closes #1016

This PR also removes all the artifact infrastructure from ClimaCoupler.

@Sbozzolo Sbozzolo force-pushed the gb/landsea branch 3 times, most recently from 1ec7ea0 to 6ac91fd Compare October 9, 2024 22:08
@Sbozzolo Sbozzolo marked this pull request as draft October 10, 2024 15:49
@szy21
Copy link
Member

szy21 commented Oct 10, 2024

We can either try using this and see if there is a problem (with our resolution I guess probably not), or use the one @akshaysridhar referenced here, which is already better than what we have now.

@akshaysridhar
Copy link
Member

akshaysridhar commented Oct 11, 2024

We can either try using this and see if there is a problem (with our resolution I guess probably not), or use the one @akshaysridhar referenced here, which is already better than what we have now.

Yeah this reference is in part based on the current AMIP documentation: (we reference that 360x180 dataset, but use a coarser 64x64 one in current main). Thanks @Sbozzolo for updating this to use the latest artifacts.

@szy21
Copy link
Member

szy21 commented Oct 11, 2024

We can either try using this and see if there is a problem (with our resolution I guess probably not), or use the one @akshaysridhar referenced here, which is already better than what we have now.

Yeah this reference is in part based on the current AMIP documentation: (we reference that 360x180 dataset, but use a coarser 64x64 one in current main). Thanks @Sbozzolo for updating this to use the latest artifacts.

This PR uses the topography artifact. Could you either check if this is ok, or add the higher resolution land sea mask to ClimaArtifact and update it in the coupler?

As added in CliMA/ClimaArtifacts#49

60 arcseconds is already pretty high resolution
The only artifact that might be needed is ETOPO for ClimaAtmos, but that
should go away soon
@Sbozzolo
Copy link
Member Author

I updated this PR to remove ArtifactWrappers and all the artifact infrastructure.

I think this can only be merged after CliMA/ClimaAtmos.jl#3378

Copy link
Member

@juliasloan25 juliasloan25 left a comment

Choose a reason for hiding this comment

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

The only remaining question is how to handle areas that are inland and below sea level, but besides that this looks good to merge

Comment on lines +20 to +22
by identifying where elevation is greater than 0. Note, this can lead to
misidentification of ocean in some areas of the globe that are inland but below
sea level (Dead Sea, Death Valley, ...).
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a plan to correct this ocean misidentification in the future?

@@ -1,8 +1,8 @@
# This file is machine-generated - editing it directly is not advised

julia_version = "1.11.1"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we want to stay at 1.11.1 unless there's a reason to go back to 1.11.0

@@ -2,7 +2,7 @@

julia_version = "1.10.5"
Copy link
Member

Choose a reason for hiding this comment

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

we should update perf/Manifest-v1.11.toml as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants