Update vector conversions #1392

sriharshakandala · 2023-07-24T18:17:15Z

Add custom CovariantVector to Contravariant123Vector transformation functions

Code follows the style guidelines OR N/A.
Unit tests are included OR N/A.
Code is exercised in an integration test OR N/A.
Documentation has been added/updated OR N/A.

sriharshakandala · 2023-07-25T22:46:32Z

bors try

bors · 2023-07-25T23:26:14Z

try

Build failed:

buildkite/climacore-ci

sriharshakandala · 2023-07-26T16:28:09Z

Split off from PR #1385

charleskawczynski

This PR looks good to me. It's a bit unfortunate that these are not captured in the axis tensor benchmarks. Can you point to the methods that these new functions are overriding / specializing on?

I'm curious if we can write a more general overloading, like we did before for project / transform. It seems that one crux of this is to cache ∂x∂ξ' * ∂x∂ξ, which seems like a good idea (I forgot that we left that on the table before).

Related issue: #867

sriharshakandala · 2023-07-26T18:01:04Z

This PR looks good to me. It's a bit unfortunate that these are not captured in the axis tensor benchmarks. Can you point to the methods that these new functions are overriding / specializing on?

I'm curious if we can write a more general overloading, like we did before for project / transform. It seems that one crux of this is to cache ∂x∂ξ' * ∂x∂ξ, which seems like a good idea (I forgot that we left that on the table before).

In the context of the GPU version, we noticed that this function uses much less register space per thread compared to the pre-existing version. Looking at the Nsight Systems profile for the pre-existing version, the fundamental bottleneck seems to be register space. This helps us use more threads per block or enable the GPU to schedule multiple blocks on the same SM resulting in significant speedup.

sriharshakandala · 2023-07-26T18:21:13Z

This PR looks good to me. It's a bit unfortunate that these are not captured in the axis tensor benchmarks. Can you point to the methods that these new functions are overriding / specializing on?

I'm curious if we can write a more general overloading, like we did before for project / transform. It seems that one crux of this is to cache ∂x∂ξ' * ∂x∂ξ, which seems like a good idea (I forgot that we left that on the table before).

I believe we provide a specialization that overrides https://github.com/CliMA/ClimaCore.jl/blob/main/src/Geometry/conversions.jl#L102 for this specific case. Please correct me if I am missing something!

Add specializations for CovariantVector to Contravariant123Vector transformation functions

sriharshakandala · 2023-07-26T18:54:41Z

bors try

bors · 2023-07-26T19:52:58Z

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

charleskawczynski · 2023-07-26T20:16:45Z

https://github.com/CliMA/ClimaCore.jl/blob/main/src/Geometry/conversions.jl#L102 for this specific case. Please correct me if I am missing something!

Ah, yeah, that's a pretty bad method (it calls project twice). In fact, all of the generic ones look pretty bad (cc @bloops)

https://github.com/CliMA/ClimaCore.jl/blob/main/src/Geometry/conversions.jl#L73-L109

src/Geometry/conversions.jl

charleskawczynski

This is fine for now, but we should really improving the performance of these operators more systematically (like we did with project / transform).

And add unit tests (I think GFlops.jl would work well here).

sriharshakandala · 2023-07-26T22:07:27Z

bors r+

bors · 2023-07-26T23:05:00Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

charleskawczynski · 2023-07-27T16:39:58Z

src/Geometry/conversions.jl

+Contravariant123Vector(
+    u::CovariantVector{T, (3,)},
+    local_geometry::LocalGeometry{(1, 2, 3)},
+) where {T} =
+    local_geometry.gⁱʲ * Covariant123Vector(zero(u[1]), zero(u[1]), u[1])


@sriharshakandala, can we hand-write this and avoid the dense mul?

1922: Update manifest files r=charleskawczynski a=charleskawczynski This PR updates the dependencies, which includes CliMA/ClimaCore.jl#1392 Co-authored-by: Charles Kawczynski <[email protected]>

sriharshakandala added the Work-in-progress label Jul 24, 2023

sriharshakandala force-pushed the sk/update_conversions branch from 2a86734 to 52ab498 Compare July 25, 2023 22:00

bors bot added a commit that referenced this pull request Jul 25, 2023

Try #1392:

d4d9836

sriharshakandala removed the Work-in-progress label Jul 25, 2023

sriharshakandala marked this pull request as ready for review July 25, 2023 23:20

sriharshakandala requested review from charleskawczynski and simonbyrne July 25, 2023 23:20

sriharshakandala changed the title ~~Update tensor conversions~~ Update vector conversions Jul 25, 2023

sriharshakandala force-pushed the sk/update_conversions branch from 52ab498 to df7d4ea Compare July 26, 2023 00:55

charleskawczynski reviewed Jul 26, 2023

View reviewed changes

Add g_{ij} metric tensor to local_geometry.

d315e51

Add specializations for CovariantVector to Contravariant123Vector transformation functions

sriharshakandala force-pushed the sk/update_conversions branch from efc7ac4 to d315e51 Compare July 26, 2023 18:54

bors bot added a commit that referenced this pull request Jul 26, 2023

Try #1392:

b784cfc

charleskawczynski reviewed Jul 26, 2023

View reviewed changes

src/Geometry/conversions.jl Show resolved Hide resolved

charleskawczynski approved these changes Jul 26, 2023

View reviewed changes

bors bot merged commit d893ee6 into main Jul 26, 2023
6 checks passed

bors bot deleted the sk/update_conversions branch July 26, 2023 23:05

charleskawczynski reviewed Jul 27, 2023

View reviewed changes

charleskawczynski mentioned this pull request Jul 27, 2023

Update manifest files CliMA/ClimaAtmos.jl#1922

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update vector conversions #1392

Update vector conversions #1392

sriharshakandala commented Jul 24, 2023 •

edited

Loading

sriharshakandala commented Jul 25, 2023

bors bot commented Jul 25, 2023

sriharshakandala commented Jul 26, 2023

charleskawczynski left a comment •

edited

Loading

sriharshakandala commented Jul 26, 2023

sriharshakandala commented Jul 26, 2023 •

edited

Loading

sriharshakandala commented Jul 26, 2023

bors bot commented Jul 26, 2023

charleskawczynski commented Jul 26, 2023

charleskawczynski left a comment •

edited

Loading

sriharshakandala commented Jul 26, 2023

bors bot commented Jul 26, 2023

charleskawczynski Jul 27, 2023

Update vector conversions #1392

Update vector conversions #1392

Conversation

sriharshakandala commented Jul 24, 2023 • edited Loading

sriharshakandala commented Jul 25, 2023

bors bot commented Jul 25, 2023

try

sriharshakandala commented Jul 26, 2023

charleskawczynski left a comment • edited Loading

Choose a reason for hiding this comment

sriharshakandala commented Jul 26, 2023

sriharshakandala commented Jul 26, 2023 • edited Loading

sriharshakandala commented Jul 26, 2023

bors bot commented Jul 26, 2023

try

charleskawczynski commented Jul 26, 2023

charleskawczynski left a comment • edited Loading

Choose a reason for hiding this comment

sriharshakandala commented Jul 26, 2023

bors bot commented Jul 26, 2023

charleskawczynski Jul 27, 2023

Choose a reason for hiding this comment

sriharshakandala commented Jul 24, 2023 •

edited

Loading

charleskawczynski left a comment •

edited

Loading

sriharshakandala commented Jul 26, 2023 •

edited

Loading

charleskawczynski left a comment •

edited

Loading