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

Adds kernel computed field #1293

Merged
merged 10 commits into from
Jan 23, 2021
Merged

Conversation

tomchor
Copy link
Collaborator

@tomchor tomchor commented Jan 7, 2021

Adds KernelComputedField <: AbstractField which is computed using a user-defined @kernel function via KernelAbstractions.jl. To do:

  • Implementation
  • Testing
  • Example / Docs
  • Test complex examples on GPUs

Closes #1246

@ali-ramadhan
Copy link
Member

I think GitHub lets us convert PRs to "draft PRs" which could be an alternative to putting "[WIP]" in the PR title.

image

@tomchor
Copy link
Collaborator Author

tomchor commented Jan 13, 2021

@glwagner Sorry for posting in the wrong place. Here's the same message again:

@glwagner I modified the docstring with a small example that I think illustrates the patterns relatively in this commit. I'm assuming that's what you meant by "example", right?

I've tested it already in some simple examples and it produces correct results. Is there anything else left besides testing this with an actual complex calculation that can only be done via KernelComputedField on a GPU?

@tomchor
Copy link
Collaborator Author

tomchor commented Jan 14, 2021

I already tested it for some complex calculations (TKE, Ri, Ro and Ertel PV) and the results are correct!

@tomchor tomchor changed the title [WIP] Adds kernel computed field Adds kernel computed field Jan 20, 2021
Copy link
Member

@ali-ramadhan ali-ramadhan left a comment

Choose a reason for hiding this comment

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

Looks great! I'm guessing this would simplify some of the statistics in LESbrary.jl?

using Oceananigans.Grids: Cell, Face

@inline ψ²(i, j, k, grid, ψ, Ψ) = @inbounds (ψ[i, j, k] - Ψ[i, j, k])^2
@kernel function compute_var!(var, grid, ϕ, Φ)
Copy link
Member

Choose a reason for hiding this comment

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

I know it's just an example but would compute_perturbation! be a better name?

Copy link
Collaborator Author

@tomchor tomchor Jan 23, 2021

Choose a reason for hiding this comment

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

I'm open to a change of names, but my idea was that it computes the variance, hence compute_var!. (Although in hindsight I should have just named it compute_variance! and be more explicit.)

Copy link
Member

@ali-ramadhan ali-ramadhan Jan 23, 2021

Choose a reason for hiding this comment

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

Ah haha sorry I thought it was short for compute_variable!. Indeed compute_variance! would be clearer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@ali-ramadhan ali-ramadhan merged commit 50b9413 into CliMA:master Jan 23, 2021
@ali-ramadhan
Copy link
Member

@tomchor Just merged it! We should add you as a collaborator on the repo so you can merge your own PRs once they're approved.

@tomchor tomchor deleted the tc/kernelcomputedfield branch January 24, 2021 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abstraction for using "custom" kernels to compute fields (plus an example)?
3 participants