Remove Adapt dependence, use ETOPO2022 dataset and regrid orography using SpaceVaryingInputs #3378

akshaysridhar · 2024-10-10T17:00:06Z

No description provided.

akshaysridhar · 2024-10-10T17:55:41Z

Sbozzolo · 2024-10-10T18:26:50Z

If is no longer needed, can also remove the gaussian_smooth, its tests, and the dependency on Distributions in test?

szy21 · 2024-10-10T18:43:16Z

Could you make the x-axis the same as the one in the NCAR paper? We can then compare the results more carefully.

examples/topography_spectra.jl

src/utils/AtmosArtifacts.jl

src/utils/common_spaces.jl

szy21

Thanks! Here are some comments. And I agree that some documentation would be helpful.

src/solver/type_getters.jl

src/utils/common_spaces.jl

examples/topography_spectra.jl

examples/Manifest.toml

src/solver/type_getters.jl

src/utils/common_spaces.jl

src/solver/type_getters.jl

Sbozzolo · 2024-10-22T17:21:42Z

@akshaysridhar could also remove the ETOPO1 artifact as part of this pull request?

If it is still needed for tests, could you ensure that it is only downloaded in tests?

I think this is the only artifact left that ClimaCoupler has manually download, everything else is handled automatically.

juliasloan25 · 2024-10-24T18:13:22Z

Is this PR ready to merge?

Sbozzolo · 2024-10-24T22:25:56Z

Thank you!

akshaysridhar · 2024-10-25T17:09:20Z

@Sbozzolo @szy21 compute hmax in the gravity wave parameterization is expensive with the new dataset (ci timeouts) so I added a simple downscaled array (Skip nth pt in data so array size is 3600x1800).

examples/topography_spectra.jl

src/utils/common_spaces.jl

Sbozzolo · 2024-10-28T14:56:43Z

The failing tests are for the restart tests. integrator.p.precomputed.sfc_conditions.obukhov_length is found to be NaN.

What has changed with respect to this?

(Also, this PR can remove Adapt from the main Project.toml)

akshaysridhar · 2024-10-28T16:29:31Z

The failing tests are for the restart tests. integrator.p.precomputed.sfc_conditions.obukhov_length is found to be NaN.

What has changed with respect to this?

(Also, this PR can remove Adapt from the main Project.toml)

I do not expect changes in this PR to affect the surface_setup cache construction (longrun outputs are expected to vary since we are modifying topography smoothing parameters) - I've attempted to manually recreate this restart test with a longrun config setup; setting topography= "Earth", rad=nothing, turbconv=nothing to mimic the (failing) restart test config dict - this check runs successfully; perhaps we can take a look offline.

szy21 · 2024-10-28T20:17:15Z

Which one is the parameter / config that allows the user to choose damping factor? Or are you going to add it in the next PR?

akshaysridhar · 2024-10-28T20:30:58Z

Which one is the parameter / config that allows the user to choose damping factor? Or are you going to add it in the next PR?

In this instance, there is a parameter n_attenuation ; the entire smoothing method should, in my opinion, be in ClimaCore which requires an additional package release + update so I want to get the artifact changes with a simple implementation into ClimaAtmos through this PR first.

szy21 · 2024-10-28T20:34:13Z

Which one is the parameter / config that allows the user to choose damping factor? Or are you going to add it in the next PR?

In this instance, there is a parameter n_attenuation ; the entire smoothing method should, in my opinion, be in ClimaCore which requires an additional package release + update so I want to get the artifact changes with a simple implementation into ClimaAtmos through this PR first.

But n_attenuation is hard coded and not in config yet? I'm fine with getting this in, just to make sure.

akshaysridhar · 2024-10-28T20:35:16Z

Which one is the parameter / config that allows the user to choose damping factor? Or are you going to add it in the next PR?

In this instance, there is a parameter n_attenuation ; the entire smoothing method should, in my opinion, be in ClimaCore which requires an additional package release + update so I want to get the artifact changes with a simple implementation into ClimaAtmos through this PR first.

But n_attenuation is hard coded and not in config yet? I'm fine with getting this in, just to make sure.

Right, it is currently defined only within the common_spaces util

akshaysridhar · 2024-10-29T20:50:57Z

Which one is the parameter / config that allows the user to choose damping factor? Or are you going to add it in the next PR?

In this instance, there is a parameter n_attenuation ; the entire smoothing method should, in my opinion, be in ClimaCore which requires an additional package release + update so I want to get the artifact changes with a simple implementation into ClimaAtmos through this PR first.

But n_attenuation is hard coded and not in config yet? I'm fine with getting this in, just to make sure.

Right, it is currently defined only within the common_spaces util

@szy21: With the restart check updated in this commit, I've also added the damping factor to the config arg list.

test/restart.jl

Replace interpolation op with generalised spacevaryingutility tools. Update orographic gravity wave file to use new artifact. Downsample raw data for use in parameterization (raw size 21600x10800 is too large). Bump ref counter and update news Co-authored-by: Gabriele Bozzola <[email protected]>

akshaysridhar force-pushed the as/topo-spacevarutil branch 4 times, most recently from b5adf54 to 691e698 Compare October 10, 2024 17:50

akshaysridhar force-pushed the as/topo-spacevarutil branch 3 times, most recently from 9e868d3 to 2a44075 Compare October 11, 2024 16:28

szy21 marked this pull request as ready for review October 14, 2024 18:30

szy21 requested review from Sbozzolo and szy21 October 14, 2024 18:30

Sbozzolo reviewed Oct 14, 2024

View reviewed changes

szy21 reviewed Oct 14, 2024

View reviewed changes

src/solver/type_getters.jl Outdated Show resolved Hide resolved

src/utils/common_spaces.jl Outdated Show resolved Hide resolved

examples/topography_spectra.jl Show resolved Hide resolved

akshaysridhar force-pushed the as/topo-spacevarutil branch 5 times, most recently from 8f1ea66 to 961018c Compare October 16, 2024 22:27