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

Update CUDA packages (attempt 2) #388

Merged
merged 11 commits into from
Sep 2, 2019
Merged

Update CUDA packages (attempt 2) #388

merged 11 commits into from
Sep 2, 2019

Conversation

ali-ramadhan
Copy link
Member

The main point of upgrading is to get rid of CUDA errors on the CPU and use CUDAapi.has_cuda() which replaces the HAVE_CUDA variable we've been using.

This was originally in PR #378 (this PR is mostly cherry picked commits) but when it was merged @glwagner reported issues with forcing functions on the GPU.

So now I've added a test that makes sure that forcing functions don't crash when used in a CPU or GPU model (one with and one without const).

@ali-ramadhan ali-ramadhan added the GPU 👾 Where Oceananigans gets its powers from label Aug 29, 2019
@ali-ramadhan ali-ramadhan changed the title Update CUDA packages (attempt #2) Update CUDA packages (attempt 2) Aug 29, 2019
function time_step_with_forcing_function_const(arch)
const τ = 60
Copy link
Member

Choose a reason for hiding this comment

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

ya.

@glwagner
Copy link
Member

Should also add some tests with sin and exp just to be complete? This will additionally test whether GPUifyLoops is correctly replacing these with their CUDAnative flavors.

@codecov
Copy link

codecov bot commented Sep 1, 2019

Codecov Report

Merging #388 into master will decrease coverage by 7.4%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
- Coverage   70.68%   63.28%   -7.41%     
==========================================
  Files          23       23              
  Lines        1395     1378      -17     
==========================================
- Hits          986      872     -114     
- Misses        409      506      +97
Impacted Files Coverage Δ
src/Oceananigans.jl 83.33% <100%> (-16.67%) ⬇️
src/models.jl 92.68% <100%> (ø) ⬆️
src/poisson_solvers.jl 40% <0%> (-57.62%) ⬇️
src/utils.jl 39.34% <0%> (-19.56%) ⬇️
src/fields.jl 46.83% <0%> (-15.19%) ⬇️
src/time_steppers.jl 74.19% <0%> (-1.94%) ⬇️
src/boundary_conditions.jl 62.5% <0%> (-1.79%) ⬇️
src/output_writers.jl 76.06% <0%> (-0.54%) ⬇️
src/halo_regions.jl 86% <0%> (-0.28%) ⬇️

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 af5d513...e113346. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 1, 2019

Codecov Report

Merging #388 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
- Coverage   70.68%   70.63%   -0.05%     
==========================================
  Files          23       23              
  Lines        1395     1597     +202     
==========================================
+ Hits          986     1128     +142     
- Misses        409      469      +60
Impacted Files Coverage Δ
src/Oceananigans.jl 100% <100%> (ø) ⬆️
src/models.jl 92.68% <100%> (ø) ⬆️
src/utils.jl 58.13% <0%> (-0.77%) ⬇️
src/output_writers.jl 78.24% <0%> (+1.65%) ⬆️
src/planetary_constants.jl 15.21% <0%> (+2.31%) ⬆️
src/diagnostics.jl 86.27% <0%> (+2.49%) ⬆️

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 af5d513...ec6b13a. Read the comment docs.

@ali-ramadhan
Copy link
Member Author

I added three forcing function + time-stepping tests to make sure it always works now. CPU and GPU tests passed on Google Cloud so all CI pipelines should pass hopefully.

@glwagner Please approve and merge if it looks okay.

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.

Thanks for updating the example too.

@glwagner glwagner merged commit e65a74a into master Sep 2, 2019
@ali-ramadhan ali-ramadhan deleted the update-cupkgs-v3 branch September 2, 2019 20:35
@ali-ramadhan ali-ramadhan added the package 📦 Quite meta label Sep 18, 2019
arcavaliere pushed a commit to arcavaliere/Oceananigans.jl that referenced this pull request Nov 6, 2019
Update CUDA packages (attempt 2)

Former-commit-id: e65a74a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GPU 👾 Where Oceananigans gets its powers from package 📦 Quite meta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants