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

Parameters field in Model for user convenience #395

Merged
merged 7 commits into from
Sep 11, 2019
Merged

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Sep 10, 2019

This PR adds a field parameters to Model. The field is intended to be utilized by users for setting parameters in user-defined forcing functions and boundary conditions functions.

The forcing functions now have the function signature

F.u(i, j, k, grid, time, U, C, params)

whereas boundary conditions have the function signature

boundary_condition(i, j, grid, time, iteration, U, C, params)

This PR resolves #394 and #390.

@codecov
Copy link

codecov bot commented Sep 10, 2019

Codecov Report

Merging #395 into master will increase coverage by 7.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
+ Coverage   64.62%   71.69%   +7.06%     
==========================================
  Files          23       23              
  Lines        1405     1413       +8     
==========================================
+ Hits          908     1013     +105     
+ Misses        497      400      -97
Impacted Files Coverage Δ
src/models.jl 92.85% <100%> (ø) ⬆️
src/boundary_conditions.jl 83.05% <100%> (+1.69%) ⬆️
src/time_steppers.jl 74.48% <100%> (+1.14%) ⬆️
src/output_writers.jl 77.71% <0%> (+0.54%) ⬆️
src/utils.jl 64.44% <0%> (+14.44%) ⬆️
src/fields.jl 62.02% <0%> (+15.18%) ⬆️
src/Oceananigans.jl 100% <0%> (+16.66%) ⬆️
src/poisson_solvers.jl 97.61% <0%> (+57.61%) ⬆️

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 079cb53...bd0ce51. Read the comment docs.

@ali-ramadhan ali-ramadhan added the feature 🌟 Something new and shiny label Sep 10, 2019
@ali-ramadhan
Copy link
Member

ali-ramadhan commented Sep 11, 2019

Performance benchmarks seem fine timing wise (model hasn't slowed down), but there seems to be some performance regression as we now perform tons of little memory allocations somewhere. I'll open an issue but this should be merged as it's not related to this PR.


CPU:

 ───────────────────────────────────────────────────────────────────────────────────────────────────────────
                 Forcing function benchmarks                        Time                   Allocations      
                                                            ──────────────────────   ───────────────────────
                      Tot / % measured:                          69.0s / 32.8%           14.0GiB / 55.9%    

 Section                                            ncalls     time   %tot     avg     alloc   %tot      avg
 ───────────────────────────────────────────────────────────────────────────────────────────────────────────
 128×128×128 with forcing (params) (CPU, Float64)       10    7.76s  34.2%   776ms   2.68GiB  34.2%   274MiB
 128×128×128 with forcing (consts) (CPU, Float64)       10    7.63s  33.7%   763ms   2.58GiB  32.9%   264MiB
 128×128×128   no forcing (CPU, Float64)                10    7.27s  32.1%   727ms   2.58GiB  32.9%   264MiB
 ───────────────────────────────────────────────────────────────────────────────────────────────────────────

GPU:

 ───────────────────────────────────────────────────────────────────────────────────────────────────────────
                 Forcing function benchmarks                        Time                   Allocations      
                                                            ──────────────────────   ───────────────────────
                      Tot / % measured:                          69.6s / 0.21%           9.23GiB / 0.30%    

 Section                                            ncalls     time   %tot     avg     alloc   %tot      avg
 ───────────────────────────────────────────────────────────────────────────────────────────────────────────
 128×128×128 with forcing (params) (GPU, Float64)       10   48.9ms  33.7%  4.89ms   9.62MiB  33.5%  0.96MiB
 128×128×128 with forcing (consts) (GPU, Float64)       10   48.1ms  33.2%  4.81ms   9.55MiB  33.3%  0.96MiB
 128×128×128   no forcing (GPU, Float64)                10   47.9ms  33.1%  4.79ms   9.55MiB  33.3%  0.96MiB
 ───────────────────────────────────────────────────────────────────────────────────────────────────────────

@glwagner
Copy link
Member Author

we now perform tons of little memory allocations somewhere.

But only on CPU?

@glwagner glwagner merged commit 95b12b2 into master Sep 11, 2019
@glwagner glwagner deleted the parameters-field branch September 11, 2019 14:34
arcavaliere pushed a commit to arcavaliere/Oceananigans.jl that referenced this pull request Nov 6, 2019
Parameters field in Model for user convenience

Former-commit-id: 95b12b2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🌟 Something new and shiny
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convenience 'parameters' field in Model for user-defined functions
2 participants