CliMA / ClimaCore.jl

CliMA model dycore
https://clima.github.io/ClimaCore.jl/dev
Apache License 2.0
87 stars 9 forks source link

Re-design AxisTensors #867

Open charleskawczynski opened 2 years ago

charleskawczynski commented 2 years ago

Purpose

The purpose of this SDI is to improve the design of AxisTensors. There are several issues with the existing design:

  1. A single "mega-type" (a type-heavy struct) is used to allow for many different types of axis tensors-- (some previous tests create 3486 different kinds, to be exact).
  2. Due to the "mega-type" approach, we end up stuck having to generate many methods for performant runtime code, rather than using far fewer methods and relying on dispatch. One price we pay with that is significantly higher latency. #853 makes some ClimaAtmos examples go from 2 min to 7 min. But other, larger jobs, reduce in runtime by ~10%.
  3. Documentation. We can't easily document this type because there are so many potential type parameters. We sort of mask this by creating many user-facing aliases, but those are not easily documentable, and users often only see this in source code, and not in stack traces, because stack traces will show typeof(::AxisTensor) which will not show the alias, but the concrete type of AxisTensor, creating a visual mismatch for people debugging.
  4. There are many wrappers in Geometry which perform nested operations on specific types and simply promote types in order to function, at the cost of doing all operations in a dense manner. This results in a huge number of flops relative to how many may actually be needed. One key aspect of the re-design will be to offer functions that do operations with a minimal number of flops.

Link to any relevant PRs/issues.

Cost/benefits/risks

Producers

Components

Inputs

There are many relations (e.g., conversions) that will need to be re-written, but most of this is related to the type design

Results and deliverables

A better design to AxisTensors

Task breakdown

A preliminary list of PRs and a preliminary timeline of PRs, milestones, and key results.

Reviewers

tapios commented 2 years ago

I'd like to see a clear description of the minimum functionality that we need (e.g., transformation covariant -> contravariant and inverse, in the horizontal and in 3d). That is, put down a set of requirements that need to be fulfilled, and then discuss the code structure that will fulfill them.

From the description here, I have the impression that what we need is not so much an improved design of AxisTensors (how did we ever get to 3486 types?), but getting rid of it and replacing it by much simpler code. I'd be hesitant to invest resources in improving something just because we have sunken costs in it.

charleskawczynski commented 2 years ago

I'd like to see a clear description of the minimum functionality that we need (e.g., transformation covariant -> contravariant and inverse, in the horizontal and in 3d). That is, put down a set of requirements that need to be fulfilled, and then discuss the code structure that will fulfill them.

Good point. Since @simonbyrne has some ideas about this, let's wait for when he returns.

From the description here, I have the impression that what we need is not so much an improved design of AxisTensors (how did we ever get to 3486 types?), but getting rid of it and replacing it by much simpler code. I'd be hesitant to invest resources in improving something just because we have sunken costs in it.

In some ways yes. And it's not that we have 3486 types, it's that we can make 3486 (distinguishable) types. Some of these transformations are indistinguishable by changing coordinate systems. For example, transformations in a x-y plane are distinguishable from transformations in a y-z plane.

I think @simonbyrne mentioned that we may be able to continue using what we have and just forward generic transformations to more generic methods (to avoid defining many methods). This would fix the many method issue, however, it wouldn't really address the documentation issue.

On another note, timings in point 2 should be confirmed. I've since run other examples that didn't result in slowdown. It's possible that more inlining is needed.

simonbyrne commented 2 years ago

There are 3 main representations we make use of: covariant, contravariant, local (aka uvw). This means that there are 6 possible conversion directions.

What really causes the blow up is dealing with subvectors (e.g. Covariant12Vector, WVector, etc), and subspaces (e.g. shallow water equations operate in a "1,2" space, inertial gravity wave in a "1,3" space). Adding in all these combinations causes a large explosion in the number of methods we use.

Additionally, we also currently define transform and project: these are basically the same, but transform will throw an error if the conversion is not exact (i.e. if you drop a non-zero dimension).

@charleskawczynski do you have a list of the methods we generate? Can you post them in a gist so I can see what we're using? That might help narrow down the scope

charleskawczynski commented 2 years ago

@charleskawczynski do you have a list of the methods we generate? Can you post them in a gist so I can see what we're using? That might help narrow down the scope

Yes, here is the gist. It includes:

There are a lot, but as you've said, it still narrows down the scope

charleskawczynski commented 1 year ago

There are actually more methods that I missed, which call project in a nested fashion, those should probably be written to minimize loads/stores/flops, too.

charleskawczynski commented 1 year ago

For example:

transform(
    ax::ContravariantAxis,
    v::CovariantTensor,
    local_geometry::LocalGeometry,
) = project(
    ax,
    local_geometry.∂ξ∂x *
    local_geometry.∂ξ∂x' *
    project(dual(axes(local_geometry.∂ξ∂x, 1)), v),
)

This is probably very slow.