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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Use higher resolution land-sea mask #1006

wants to merge 7 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

Issues closed by this PR

High-res land/sea mask closes #936
Artifact removal closes #730 and #1016
Manifest removal in seabreeze and perf closes #1056
perf/ removal closes #1057

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?

@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

NEWS.md Show resolved Hide resolved
@@ -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

perf/Manifest.toml Outdated Show resolved Hide resolved
@Sbozzolo Sbozzolo marked this pull request as ready for review November 5, 2024 16:42
perf/flame.jl Outdated
@@ -1,6 +1,8 @@
# flame.jl: provides allocation breakdown for individual backtraces for single-process unthredded runs
# and check for overall allocation limits based on previous runs
# copied and modified from `ClimaAtmos/perf`
#
# To run this, add ProfileCanvas to the ClimaEarth environment
Copy link
Member

Choose a reason for hiding this comment

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

This should be added to flame_diff.jl and flame_test.jl too

Copy link
Member Author

Choose a reason for hiding this comment

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

They don't seem to need ProfleCanvas

Copy link
Member Author

Choose a reason for hiding this comment

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

Because they import ProfileCanvasDiff (which copies the code of ProfileCanvas)

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
@@ -48,27 +41,17 @@ steps:
- echo "--- Instantiate ClimaEarth env"
- "julia --project=experiments/ClimaEarth/ -e 'using Pkg; Pkg.develop(path=\".\")'"
- "julia --project=experiments/ClimaEarth/ -e 'using Pkg; Pkg.instantiate(;verbose=true)'"
- "julia --project=experiments/ClimaEarth/ -e 'using Pkg; Pkg.add(\"MPI\"); Pkg.add(\"CUDA\")'"
- "julia --project=experiments/ClimaEarth/ -e 'using Pkg; Pkg.add(\"MPI\"); Pkg.add(\"CUDA\");"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- "julia --project=experiments/ClimaEarth/ -e 'using Pkg; Pkg.add(\"MPI\"); Pkg.add(\"CUDA\");"
- "julia --project=experiments/ClimaEarth/ -e 'using Pkg; Pkg.add(\"MPI\"); Pkg.add(\"CUDA\");'"

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

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