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

Reduce allocations and resolution dependence in how we get surface values from a center field #615

Closed
kmdeck opened this issue May 10, 2024 · 1 comment · Fixed by #686
Closed
Assignees
Labels
SDI Software Design Issue

Comments

@kmdeck
Copy link
Member

kmdeck commented May 10, 2024

Purpose

Reduce allocations and remove duplicate code

Description

The soil model requires, when setting boundary conditions, the values of the state and certain derived properties (temperature, specific humidity, etc) at the surface.

  1. We currently use
    top_center_to_surface

    function top_center_to_surface(center_field::ClimaCore.Fields.Field)

    which takes the top center value as the surface value, and creates a field on the surface space. This is allocating, and resolution dependent since the value at the top center depends on resolution.

  2. We also have

    function get_top_surface_field(
    and
    function get_top_surface_field(center_val, _)
    .

The first method, as far as I can tell, allocates and does what #1 does, but in a cleaner way. The second method is for when we have a scalar (not depth dependent) argument and so we dont need to interpolate.

  1. In a recent PR, we introduced
    linear_interpolation_to_surface!(sfc_field, center_field, z) which allocates when we create the \Delta z field, but does not allocate the surface field otherwise (but instead modifies the values at a preallocated spot). The benefit to this is that it should be less resolution dependent.

Desired solution

  1. A single function for extrapolating from the center field values to the surface (we could support both nearest neighbor and linear extrapolation, but Im not sure we need to, as the first is resolution dependent and the latter is less so).
  2. We still need to support this handling scalar input
  3. All methods non-allocating (this will require adding scratch space) to p. This may require adding \Delta z fields and z to p as well.

Impacts

Since we use this functionality (center to surface extrapolation) a lot when computing BC, especially the "atmos driven ones" this might tie into performance work @Sbozzolo is doing on turbulent_fluxes. These same functions also will show up in the jacobian boundary contribution terms that @juliasloan25 is working on

We may also need to think about where scratch space is stored. is p.soil.sfc_scratch1, p.soil.sfc_scratch2 reasonable? or should we do p.soil.scratch.sfc1? idk. will think more and we can discuss

@kmdeck kmdeck added the SDI Software Design Issue label May 10, 2024
@kmdeck kmdeck changed the title Product title Reduce allocations and resolution dependence in how we get surface values from a center field May 10, 2024
@kmdeck kmdeck self-assigned this May 10, 2024
@kmdeck kmdeck added this to the Maintenance and Improvements milestone May 10, 2024
This was referenced May 10, 2024
This was referenced Jul 2, 2024
@kmdeck
Copy link
Member Author

kmdeck commented Jul 2, 2024

We combined the two nearest neighbor functions (1 and 2) into a single set in Domains, with a method for scalars PR #686

We still support the linear interpolation as well.

Allocations were reduced in PR #658

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SDI Software Design Issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant