Skip to content

Conversation

@ali-ramadhan
Copy link
Member

This PR fixes a recent regression in Model construction time due to broadcasting over an OffsetArray{CuArray} getting converted to a scalar operation. Constructing models should be fast again.

I've also completely disabled scalar operations on CuArrays so this shouldn't happen again.

cc @sandreza

Actually fixes #82

@ali-ramadhan ali-ramadhan added bug 🐞 Even a perfect program still has bugs performance 🏍️ So we can get the wrong answer even faster labels Jun 4, 2019
Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

👍

@glwagner
Copy link
Member

glwagner commented Jun 4, 2019

Would also be useful to have a function (ie, parentdata) that returns a view over the parent

@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #264 into master will increase coverage by 0.41%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
+ Coverage   70.73%   71.14%   +0.41%     
==========================================
  Files          23       23              
  Lines         868      870       +2     
==========================================
+ Hits          614      619       +5     
+ Misses        254      251       -3
Impacted Files Coverage Δ
src/Oceananigans.jl 71.42% <ø> (ø) ⬆️
src/models.jl 90.47% <100%> (ø) ⬆️
src/boundary_conditions.jl 78.78% <0%> (+11.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1f4c33...32fc894. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #264 into master will increase coverage by 0.29%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
+ Coverage   70.73%   71.03%   +0.29%     
==========================================
  Files          23       23              
  Lines         868      870       +2     
==========================================
+ Hits          614      618       +4     
+ Misses        254      252       -2
Impacted Files Coverage Δ
src/Oceananigans.jl 71.42% <ø> (ø) ⬆️
src/models.jl 90% <100%> (-0.48%) ⬇️
src/fields.jl 58.82% <100%> (+1.24%) ⬆️
src/planetary_constants.jl 9.67% <0%> (-3.23%) ⬇️
src/boundary_conditions.jl 78.78% <0%> (+11.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1f4c33...32fc894. Read the comment docs.

@ali-ramadhan
Copy link
Member Author

Would also be useful to have a function (ie, parentdata) that returns a view over the parent

Decided to go with underlying_data(::Field) as it's a clearer name than parent, and I think it's a little different from Base.parent, but I'm not opposed to overloading Base.parent.

Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

I suggested “parentdata” to match the semantics of OffsetArrays, but underlyingdata is ok with me.

@vchuravy
Copy link
Collaborator

vchuravy commented Jun 4, 2019

x-ref: JuliaArrays/OffsetArrays.jl#57

@ali-ramadhan ali-ramadhan merged commit 9204afe into master Jun 4, 2019
@ali-ramadhan ali-ramadhan deleted the no-scalar-ops branch June 20, 2019 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 🐞 Even a perfect program still has bugs performance 🏍️ So we can get the wrong answer even faster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disable slow fallback methods for CUDA.

4 participants