adapt functions for diagnostic_variables.jl #129

katharinamaetschke · 2022-08-26T11:53:38Z

@maximilian-gelbrecht Adapt functions for the structs in diagnostic_variables.jl in the way they were also implemented in the lower_triangular_matrices.jl file. I haven't tested them yet, but wanted to know if this is basically the right way to do it.

milankl · 2022-08-26T11:57:03Z

src/SpeedyWeather.jl

-    import KernelAbstractions 
+    using KernelAbstractions 


In general, use import PkgName: PkgName, A, B, C instead of using. Otherwise we may get conflicts when packages export functions with the same name as we want to define. Keeps our name space smaller.

The branch is WIP and we'll probably end up using almost all functions KernelAbstractions exports. While working on it, I would just keep the using statement and then in the end when merging into main change it to import the functions that we actually use

Sorry yeah, do as you like! We can also convert this PR to draft if it's not meant to be merged anytime soon

milankl · 2022-08-26T11:59:24Z

Project.toml

+Enzyme = "7da242da-08ed-463a-9acd-ee780be4f1d9"
 FFTW = "7a1cc6ca-52ef-59f5-83cd-3a7055c09341"
 FastGaussQuadrature = "442a2c76-b920-505d-bb47-c5924d526838"
 JLD2 = "033835bb-8acc-5ee8-8aae-3f567f8a3819"
 KernelAbstractions = "63c18a36-062a-441e-b654-da1e3ab1ce7c"
+KernelGradients = "e5faadeb-7f6c-408e-9747-a7a26e81c66a"


Happy for Enzyme and KernelGradients to be added, but can you also directly add some compatible versions to [compat]?

Can't we let CompatHelper handle this?

Yes, but in my experience it takes a while for CompatHelper to create a PR for this stuff. So I've been adding things manually when I include a new package and then CompatHelper suggested new versions if they become available. Maybe you are right and it's not needed, I just ended up with a lot of package downgrades when CUAKernels 0.4 was available but our compat was still "0.3"

Ah okay, that makes sense, especially with a WIP branch that will be around longer

milankl · 2022-08-26T12:07:43Z

test/kernelabstractions.jl

@@ -1,8 +1,12 @@
-using KernelAbstractions
+using KernelAbstractions    
+import Parameters: @with_kw, @unpack


I don't think we have Parameters as dependency in our tests (package dependency and test dependency aren't the same, as you may want to test against another package but the code doesn't actually depend on it). This may work in your local environment, but this may fail at CI. Note in general (since Julia v1.7) can also unpack like this

(;lmax,mmax) = m.spectral_transform

or always rename locally if you don't feel lazy

maximum_degree_l = m.spectral_transform.lmax

We can just use the first style so,

(;lmax,mmax) = m.spectral_transform

I'd say, We can add this later the kernelabstractions_adtest branch

…tests Adapt functions for diagnostic_variables.jl

milankl · 2022-08-26T12:24:28Z

This PR and #130 are now in one that want to merge directly into main. The conflict with src/diagnostic_variables.jl should be resolved at the very end as I want to merge #127 first

milankl · 2022-08-26T19:44:48Z

I've just resolved the conflicts that arose because I changed all gridded fields from Matrix to Grid<:AbstractGrid as #127 now introduces a grid-flexible spectral transform. Quick note: I usually write {NF} ... where NF in two cases.

When I want to constraint the method of a function to input arguments with the same parametric type. That means in

DiagnosticVariables(G::Geometry{NF},S::SpectralTransform{NF}) where NF = zeros(DiagnosticVariables,G,S)

Both G and S need to have the same NF. But NF isn't actually used in the function body.

If I want to use NF (here T) in the function body. Example

function clip_negatives!(A::AbstractArray{T}) where T
    @inbounds for i in eachindex(A)
        A[i] = max(A[i],zero(T))
    end
end

Otherwise I don't think there's a need to write where NF.

maximilian-gelbrecht · 2022-08-29T08:10:41Z

@milankl why do you want this to merge directly into main? I would rather not do this until the kernelabstractions version is actually working, we agreed on a separate branch for this version at first. Also please give me the chance for a code review into the kernelabstractions branch as well and don't merge it immediatly, even if it is not the main branch. Now, this is a bit messy with adding to this PR which shouldn't go into the main.

maximilian-gelbrecht · 2022-08-29T08:53:01Z

src/diagnostic_variables.jl

@@ -202,13 +243,17 @@ function Base.zeros(::Type{DiagnosticVariablesLayer},
    return DiagnosticVariablesLayer(tendencies,grid_variables,dynamics_variables,npoints)
 end

+# GPU methods for DiagnosticVariablesLayer
+function Adapt.adapt_structure(to, dvl::DiagnosticVariablesLayer)
+    DiagnosticVariablesLayer(dvl.tendencies, dvl.grid_variables, dvl.dynamics_variables, dvl.npoints)


Here, the calls to adapt are missing for tendencies, grid_variables and dynamics_variables

maximilian-gelbrecht · 2022-08-29T08:53:45Z

src/diagnostic_variables.jl

+# GPU methods for DiagnosticVariables
+function Adapt.adapt_structure(to, dv::DiagnosticVariables{NF}) where NF
+    DiagnosticVariables(Adapt.adapt(to, dv.layers), 
+                        dv.surface, dv.nlev, dv.npoints)


surface needs to be converted as well adapt

maximilian-gelbrecht · 2022-08-29T08:56:47Z

test/kernelabstractions.jl

@@ -1,8 +1,12 @@
-using KernelAbstractions
+using KernelAbstractions    
+import Parameters: @with_kw, @unpack


We can just use the first style so,

(;lmax,mmax) = m.spectral_transform

I'd say, We can add this later the kernelabstractions_adtest branch

milankl · 2022-08-29T12:30:34Z

@milankl why do you want this to merge directly into main? I would rather not do this until the kernelabstractions version is actually working, we agreed on a separate branch for this version at first. Also please give me the chance for a code review into the kernelabstractions branch as well and don't merge it immediatly, even if it is not the main branch. Now, this is a bit messy with adding to this PR which shouldn't go into the main.

Sorry my bad! Misunderstood what was supposed to go into which branch and just incorrectly merged two PRs that pointed into different branches. I'll let you merge anything kernelabstractions-related from now on! Sorry 😬

maximilian-gelbrecht · 2022-08-29T13:51:32Z

Ok to sort this out, I guess I would just close this PR and @katharinamaetschke does the corrections as another PR into the kernelabstractions_adtest branch

And for the next PRs, everything as planned also as PRs that point to the kernel abstractions_adtest

maximilian-gelbrecht · 2022-08-29T15:37:19Z

The last commit addresses the compat issues, the unpacking in the tests and using/import statements as we discussed in the reviews

@katharinamaetschke it is best when you sync your fork on the kernelabstractions_adtest branch after commits that Milan or I do (just click the button for that on the GitHub repo of your fork), otherwise you might run into some git conflicts that are always a bit nasty to solve

maximilian-gelbrecht · 2022-09-07T12:29:50Z

I'll close this PR now. @katharinamaetschke please do another PR for the corrections into the kernelabstractions_adtest branch

katharinamaetschke · 2022-09-08T06:44:46Z

I'll close this PR now. @katharinamaetschke please do another PR for the corrections into the kernelabstractions_adtest branch

Yes, I willl, I am very sorry for taking so long!

test basics and first example for KA

dab7b55

katharinamaetschke closed this Aug 26, 2022

Adapt functions for diagnostic_variables.jl

09b6cbc

milankl reviewed Aug 26, 2022

View reviewed changes

milankl reopened this Aug 26, 2022

milankl mentioned this pull request Aug 26, 2022

Adapt functions for diagnostic_variables.jl #130

Merged

Merge pull request #130 from katharinamaetschke/kernelabstractions_ad…

62db2dd

…tests Adapt functions for diagnostic_variables.jl

Merge branch 'main' into kernelabstractions_adtests

0315062

maximilian-gelbrecht reviewed Aug 29, 2022

View reviewed changes

milankl added the gpu 🖼️ Everthing GPU related label Aug 29, 2022

adjust tests, compat and imports

f049c38

ArrayType instead of DeviceArrayNotAdapt

bd0bc54

maximilian-gelbrecht closed this Sep 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adapt functions for diagnostic_variables.jl #129

adapt functions for diagnostic_variables.jl #129

katharinamaetschke commented Aug 26, 2022

milankl Aug 26, 2022

maximilian-gelbrecht Aug 29, 2022 •

edited

Loading

milankl Aug 29, 2022

milankl Aug 26, 2022

maximilian-gelbrecht Aug 29, 2022

milankl Aug 29, 2022

maximilian-gelbrecht Aug 29, 2022

milankl Aug 26, 2022

maximilian-gelbrecht Aug 29, 2022 •

edited

Loading

milankl commented Aug 26, 2022

milankl commented Aug 26, 2022

maximilian-gelbrecht commented Aug 29, 2022 •

edited

Loading

maximilian-gelbrecht Aug 29, 2022

maximilian-gelbrecht Aug 29, 2022

maximilian-gelbrecht Aug 29, 2022 •

edited

Loading

milankl commented Aug 29, 2022

maximilian-gelbrecht commented Aug 29, 2022 •

edited

Loading

maximilian-gelbrecht commented Aug 29, 2022 •

edited

Loading

maximilian-gelbrecht commented Sep 7, 2022

katharinamaetschke commented Sep 8, 2022

adapt functions for diagnostic_variables.jl #129

adapt functions for diagnostic_variables.jl #129

Conversation

katharinamaetschke commented Aug 26, 2022

Choose a reason for hiding this comment

maximilian-gelbrecht Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilian-gelbrecht Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

milankl commented Aug 26, 2022

milankl commented Aug 26, 2022

maximilian-gelbrecht commented Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilian-gelbrecht Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

milankl commented Aug 29, 2022

maximilian-gelbrecht commented Aug 29, 2022 • edited Loading

maximilian-gelbrecht commented Aug 29, 2022 • edited Loading

maximilian-gelbrecht commented Sep 7, 2022

katharinamaetschke commented Sep 8, 2022

maximilian-gelbrecht Aug 29, 2022 •

edited

Loading

maximilian-gelbrecht Aug 29, 2022 •

edited

Loading

maximilian-gelbrecht commented Aug 29, 2022 •

edited

Loading

maximilian-gelbrecht Aug 29, 2022 •

edited

Loading

maximilian-gelbrecht commented Aug 29, 2022 •

edited

Loading

maximilian-gelbrecht commented Aug 29, 2022 •

edited

Loading