Remove implicit broadcast for `Grad` #1458

jkozdon · 2020-08-20T20:02:02Z

We want to disallow the following:

using StaticArrays
using ClimateMachine.VariableTemplates

function vs(FT)
  @vars begin
    a::FT
  end
end

FT = Float64
arr = fill!(MArray{Tuple{3, 1}, FT}(undef), 0)
g = Grad{vs(FT)}(arr)

g.a = 4           # changes arr to [4 4 4]
g.a = [5]         # changes arr to [5 5 5]

This is not a perfect solution though, because it will result in the sort of unclear error:

julia> g.a = 4
ERROR: type Grad has no field a
Stacktrace:
 [1] setproperty!(::Grad{NamedTuple{(:a,),Tuple{Float64}},MArray{Tuple{3,1},Float64,2,3},0}, ::Symbol, ::Int64) at ./Base.jl:34
 [2] top-level scope at REPL[7]:1

close #1457

@vars

We want to disallow the following: ```julia using StaticArrays using ClimateMachine.VariableTemplates function vs(FT) @vars begin a::FT end end FT = Float64 arr = fill!(MArray{Tuple{3, 1}, FT}(undef), 0) g = Grad{vs(FT)}(arr) g.a = 4 # changes arr to [4 4 4] g.a = [5] # changes arr to [5 5 5] ```

jkozdon · 2020-08-20T20:02:10Z

bors try

jkozdon · 2020-08-20T20:38:31Z

bors try

bors · 2020-08-20T20:38:32Z

try

Already running a review

bors · 2020-08-20T23:15:29Z

try

Build succeeded:

jkozdon · 2020-08-20T23:41:57Z

So this didn't break anything...

jkozdon · 2020-08-20T23:46:14Z

Not sure if anyone has any better solutions / suggestions

cc: @vchuravy, @kpamnany, @simonbyrne, @jakebolewski, @leios

kpamnany · 2020-08-21T00:07:26Z

I read the issue and your presentation of the options was clear. I think the proposed choice was the best of the options -- unintended behavior is a terrible thing. Good find!

charleskawczynski

I would be great to merge 👍

charleskawczynski · 2020-08-27T14:21:28Z

I just realized that I'm getting an error with these changes in the EDMF branch:

function flux_first_order!(
    turbconv::EDMF{FT},
    m::AtmosModel{FT},
    flux::Grad,
    state::Vars,
    aux::Vars,
    t::Real,
) where {FT}

    up_f = flux.turbconv.updraft
    ...
    for i in 1:N_up
        ρa_i = enforce_unit_bounds(up[i].ρa)
        up_f[i].ρa .= up[i].ρaw #            <-------line 855 in edmf_kernels.jl
    end

Stack trace:

ERROR: LoadError: TaskFailedException:
TaskFailedException:
setindex!(::SArray{Tuple{3},Float64,1,3}, value, ::Int) is not defined.
Stacktrace:
 [1] error at ./error.jl:33 [inlined]
 [2] setindex! at /Users/charliekawczynski/.julia/packages/StaticArrays/l7lu2/src/indexing.jl:3 [inlined]
 [3] macro expansion at /Users/charliekawczynski/.julia/packages/StaticArrays/l7lu2/src/arraymath.jl:105 [inlined]
 [4] _fill! at /Users/charliekawczynski/.julia/packages/StaticArrays/l7lu2/src/arraymath.jl:101 [inlined]
 [5] fill! at /Users/charliekawczynski/.julia/packages/StaticArrays/l7lu2/src/arraymath.jl:99 [inlined]
 [6] copyto! at ./broadcast.jl:872 [inlined]
 [7] materialize! at ./broadcast.jl:823 [inlined]
 [8] flux_first_order! at /Users/charliekawczynski/Dropbox/Caltech/work/dev/CliMA/ClimateMachine.jl/test/Atmos/EDMF/edmf_kernels.jl:855 [inlined]

Ah, but this is precisely the issue. Let me see if I can wrap the RHS with SVector. Although, the RHS is a scalar, and based on the example it looks like this should still work (since I've explicitly included a broadcast)? It does work if I multiply the RHS by the vertical unit vector. That seems a simple enough solution, and explicitly clear.

jkozdon · 2020-08-27T21:11:35Z

Ah, but this is precisely the issue. Let me see if I can wrap the RHS with SVector. Although, the RHS is a scalar, and based on the example it looks like this should still work (since I've explicitly included a broadcast)? It does work if I multiply the RHS by the vertical unit vector. That seems a simple enough solution, and explicitly clear.

I thought that this was the bug we wanted to avoid?

charleskawczynski · 2020-08-27T21:31:09Z

I thought that this was the bug we wanted to avoid?

Yes, it is. The PR is fine and does fix the problem. I was just confused at first by the error 😄

jkozdon · 2020-08-27T22:18:08Z

Cool. I'm fine with this being merged unless @simonbyrne wants to comment.

simonbyrne · 2020-08-27T22:21:39Z

bors r+

bors · 2020-08-28T00:17:34Z

Build succeeded:

1537: Use static range in Grad setproperty! r=mwarusz a=mwarusz # Description After #1458 I noticed a slowdown. This PR proposes a fix. Running ```julia --project=. experiments/AtmosLES/bomex_les.jl --monitor-timestep-duration 100steps``` I get Before: ``` ┌ Info: Wall-clock time per time-step (statistics across MPI ranks) │ maximum (s) = 1.5273617989999998e-02 │ minimum (s) = 1.5273617989999998e-02 │ median (s) = 1.5273617989999998e-02 └ std (s) = NaN ``` After: ``` Info: Wall-clock time per time-step (statistics across MPI ranks) │ maximum (s) = 1.1554147779999999e-02 │ minimum (s) = 1.1554147779999999e-02 │ median (s) = 1.1554147779999999e-02 └ std (s) = NaN ``` The fix relies on unexported `StaticArrays` API but JuliaArrays/StaticArrays.jl#474 makes me think that this is ok. 1538: Rename step (intrinsic func) r=charleskawczynski a=charleskawczynski # Description Closes #95. Co-authored-by: Maciej Waruszewski <[email protected]> Co-authored-by: Charles Kawczynski <[email protected]>

1537: Use static range in Grad setproperty! r=mwarusz a=mwarusz # Description After #1458 I noticed a slowdown. This PR proposes a fix. Running ```julia --project=. experiments/AtmosLES/bomex_les.jl --monitor-timestep-duration 100steps``` I get Before: ``` ┌ Info: Wall-clock time per time-step (statistics across MPI ranks) │ maximum (s) = 1.5273617989999998e-02 │ minimum (s) = 1.5273617989999998e-02 │ median (s) = 1.5273617989999998e-02 └ std (s) = NaN ``` After: ``` Info: Wall-clock time per time-step (statistics across MPI ranks) │ maximum (s) = 1.1554147779999999e-02 │ minimum (s) = 1.1554147779999999e-02 │ median (s) = 1.1554147779999999e-02 └ std (s) = NaN ``` The fix relies on unexported `StaticArrays` API but JuliaArrays/StaticArrays.jl#474 makes me think that this is ok. Co-authored-by: Maciej Waruszewski <[email protected]>

bors bot added a commit that referenced this pull request Aug 20, 2020

Try #1458:

e49d729

jkozdon requested a review from simonbyrne August 20, 2020 23:44

charleskawczynski approved these changes Aug 25, 2020

View reviewed changes

jkozdon marked this pull request as ready for review August 26, 2020 23:00

jkozdon linked an issue Aug 27, 2020 that may be closed by this pull request

implicit broadcast in setproperty! of Grad #1457

Closed

bors bot merged commit c5a28ff into master Aug 28, 2020

bors bot deleted the jek/gradbroadcast branch August 28, 2020 00:17

mwarusz mentioned this pull request Sep 10, 2020

Use static range in Grad setproperty! #1537

Merged

7 tasks

Remove implicit broadcast for Grad #1458

Remove implicit broadcast for Grad #1458

Uh oh!

Conversation

jkozdon commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkozdon commented Aug 20, 2020

Uh oh!

jkozdon commented Aug 20, 2020

Uh oh!

bors bot commented Aug 20, 2020

try

Uh oh!

bors bot commented Aug 20, 2020

try

Uh oh!

jkozdon commented Aug 20, 2020

Uh oh!

jkozdon commented Aug 20, 2020

Uh oh!

kpamnany commented Aug 21, 2020

Uh oh!

charleskawczynski left a comment

Choose a reason for hiding this comment

Uh oh!

charleskawczynski commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkozdon commented Aug 27, 2020

Uh oh!

charleskawczynski commented Aug 27, 2020

Uh oh!

jkozdon commented Aug 27, 2020

Uh oh!

simonbyrne commented Aug 27, 2020

Uh oh!

bors bot commented Aug 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Remove implicit broadcast for `Grad` #1458

Remove implicit broadcast for `Grad` #1458

jkozdon commented Aug 20, 2020 •

edited

Loading

charleskawczynski commented Aug 27, 2020 •

edited

Loading