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

Time step with predictor velocities #666

Merged
merged 10 commits into from
Mar 5, 2020

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Mar 5, 2020

This PR refactors the time-stepping algorithm to use the "predictor velocities" abstraction in the fractional step method. This brings the implemented algorithm in line with the algorithm described in the documentation.

@glwagner
Copy link
Member Author

glwagner commented Mar 5, 2020

@ali-ramadhan I may need some help with the checkpointer...

@ali-ramadhan
Copy link
Member

I'll have a look

if solver.type isa Channel && arch isa GPU
ϕ = RHS = solver.storage.storage1
else
ϕ = RHS = solver.storage
end

@launch(device(arch), config=launch_config(grid, :xyz),
calculate_pressure_right_hand_side!(RHS, solver.type, arch, grid, U, G, Δt))
calculate_pressure_right_hand_side!(RHS, solver.type, arch, grid, Δt, U★))

solve_poisson_equation!(solver, grid)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why but one of the Travis builds hits a ReadOnlyMemoryError on this line and I get a segmentation fault on my computer at this line.

Perhaps part of the model is corrupted after restoring from checkpoint? I'll try to find out.

Travis build log: https://travis-ci.com/climate-machine/Oceananigans.jl/jobs/294185099#L453

@ali-ramadhan
Copy link
Member

Think I fixed it. At least checkpointer tests pass on my laptop.

@codecov
Copy link

codecov bot commented Mar 5, 2020

Codecov Report

Merging #666 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #666     +/-   ##
=========================================
+ Coverage   78.28%   78.38%   +0.1%     
=========================================
  Files         118      119      +1     
  Lines        2390     2383      -7     
=========================================
- Hits         1871     1868      -3     
+ Misses        519      515      -4
Impacted Files Coverage Δ
src/TimeSteppers/time_stepping_kernels.jl 100% <ø> (ø)
src/Fields/Fields.jl 100% <ø> (ø) ⬆️
src/TimeSteppers/TimeSteppers.jl 100% <ø> (+3.7%) ⬆️
src/OutputWriters/checkpointer.jl 91.07% <ø> (ø) ⬆️
src/Models/incompressible_model.jl 88.88% <100%> (ø) ⬆️
src/TimeSteppers/generic_time_stepping.jl 100% <100%> (ø)
src/Solvers/solve_for_pressure.jl 93.33% <100%> (ø) ⬆️
src/TimeSteppers/adams_bashforth.jl 100% <100%> (ø) ⬆️
src/Architectures.jl 88.88% <0%> (-11.12%) ⬇️

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 d614833...f92b813. Read the comment docs.

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.

Performance seems unaffected although we should be careful about CPU allocations, they seems to have gotten out of hand (same on master so this PR isn't at fault):

Julia Version 1.3.1
Commit 2d5741174c (2019-12-30 21:36 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Xeon(R) Silver 4214 CPU @ 2.20GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.1 (ORCJIT, skylake)
  GPU: TITAN V

 ──────────────────────────────────────────────────────────────────────────────────────
        Static ocean benchmarks                Time                   Allocations      
                                       ──────────────────────   ───────────────────────
           Tot / % measured:                 173s / 51.2%           43.6GiB / 64.5%    

 Section                       ncalls     time   %tot     avg     alloc   %tot      avg
 ──────────────────────────────────────────────────────────────────────────────────────
  32× 32× 32  [CPU, Float32]       10    127ms  0.14%  12.7ms    170MiB  0.59%  17.0MiB
  32× 32× 32  [CPU, Float64]       10    153ms  0.17%  15.3ms    170MiB  0.59%  17.0MiB
  32× 32× 32  [GPU, Float32]       10   24.4ms  0.03%  2.44ms   10.0MiB  0.03%  1.00MiB
  32× 32× 32  [GPU, Float64]       10   24.2ms  0.03%  2.42ms   10.0MiB  0.03%  1.00MiB
  64× 64× 64  [CPU, Float32]       10    713ms  0.81%  71.3ms    676MiB  2.35%  67.6MiB
  64× 64× 64  [CPU, Float64]       10    868ms  0.98%  86.8ms    676MiB  2.35%  67.6MiB
  64× 64× 64  [GPU, Float32]       10   24.8ms  0.03%  2.48ms   10.0MiB  0.03%  1.00MiB
  64× 64× 64  [GPU, Float64]       10   25.2ms  0.03%  2.52ms   10.0MiB  0.03%  1.00MiB
 128×128×128  [CPU, Float32]       10    5.22s  5.90%   522ms   2.64GiB  9.39%   270MiB
 128×128×128  [CPU, Float64]       10    5.44s  6.14%   544ms   2.64GiB  9.39%   270MiB
 128×128×128  [GPU, Float32]       10   46.3ms  0.05%  4.63ms   10.0MiB  0.03%  1.00MiB
 128×128×128  [GPU, Float64]       10   45.6ms  0.05%  4.56ms   10.0MiB  0.03%  1.00MiB
 256×256×256  [CPU, Float32]       10    37.4s  42.3%   3.74s   10.5GiB  37.5%  1.05GiB
 256×256×256  [CPU, Float64]       10    37.7s  42.6%   3.77s   10.5GiB  37.5%  1.05GiB
 256×256×256  [GPU, Float32]       10    338ms  0.38%  33.8ms   10.0MiB  0.03%  1.00MiB
 256×256×256  [GPU, Float64]       10    336ms  0.38%  33.6ms   10.0MiB  0.03%  1.00MiB
 ──────────────────────────────────────────────────────────────────────────────────────

@glwagner glwagner merged commit 162f197 into master Mar 5, 2020
@glwagner glwagner deleted the glw/time-step-with-predictor-velocities branch March 5, 2020 05:08
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.

2 participants