OceanBioME / OceanBioME.jl

🌊 🦠 🌿 A fast and flexible modelling environment written in Julia for modelling the coupled interactions between ocean biogeochemistry, carbonate chemistry, and physics
https://oceanbiome.github.io/OceanBioME.jl/
MIT License
48 stars 21 forks source link

Reduce allocations #107

Closed navidcy closed 1 year ago

navidcy commented 1 year ago

Resolves #104

@jagoosw, are the column_diffusion_timescale(model) and the update_TwoBandPhotosyntheticallyActiveRatiation! being tested?

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 48.00% and project coverage change: +0.67 :tada:

Comparison is base (f3d1724) 52.24% compared to head (8b082db) 52.91%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #107 +/- ## ========================================== + Coverage 52.24% 52.91% +0.67% ========================================== Files 26 26 Lines 1026 1030 +4 ========================================== + Hits 536 545 +9 + Misses 490 485 -5 ``` | [Impacted Files](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/107?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME) | Coverage Δ | | |---|---|---| | [src/Light/morel.jl](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/107?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME#diff-c3JjL0xpZ2h0L21vcmVsLmps) | `0.00% <0.00%> (ø)` | | | [src/Models/Models.jl](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/107?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME#diff-c3JjL01vZGVscy9Nb2RlbHMuamw=) | `0.00% <0.00%> (ø)` | | | [src/OceanBioME.jl](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/107?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME#diff-c3JjL09jZWFuQmlvTUUuamw=) | `0.00% <ø> (ø)` | | | [src/Utils/timestep.jl](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/107?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME#diff-c3JjL1V0aWxzL3RpbWVzdGVwLmps) | `81.81% <75.00%> (+81.81%)` | :arrow_up: | | [src/Light/2band.jl](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/107?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME#diff-c3JjL0xpZ2h0LzJiYW5kLmps) | `86.48% <100.00%> (ø)` | | | [src/Models/AdvectedPopulations/LOBSTER/core.jl](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/107?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME#diff-c3JjL01vZGVscy9BZHZlY3RlZFBvcHVsYXRpb25zL0xPQlNURVIvY29yZS5qbA==) | `100.00% <100.00%> (ø)` | | | [src/Models/Individuals/SLatissima.jl](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/107?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME#diff-c3JjL01vZGVscy9JbmRpdmlkdWFscy9TTGF0aXNzaW1hLmps) | `84.17% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jagoosw commented 1 year ago

update_TwoBandPhotosyntheticallyActiveRatiation! is tested in test/test_light.jl but I don't think the timescale one is.

navidcy commented 1 year ago

OK, here are some benchmarks:

I run the following

using Oceananigans, OceanBioME
using Oceananigans.Units
const year = years = 365days
@inline PAR⁰(x, y, t) = 60 * (1 - cos((t + 15days) * 2π / year)) * (1 / (1 + 0.2 * exp(-((mod(t, year) - 200days) / 50days)^2))) + 2
@inline H(t, t₀, t₁) = ifelse(t₀ < t < t₁, 1.0, 0.0)
@inline fmld1(t) = H(t, 50days, year) * (1 / (1 + exp(-(t - 100days) / 5days))) * (1 / (1 + exp((t - 330days) / 25days)))
@inline MLD(t) = - (10 + 340 * (1 - fmld1(year - eps(year)) * exp(-mod(t, year) / 25days) - fmld1(mod(t, year))))
@inline κₜ(x, y, z, t) = 1e-2 * (1 + tanh((z - MLD(t)) / 10)) / 2 + 1e-4
@inline temp(x, y, z, t) = 2.4 * cos(t * 2π / year + 50days) + 10
grid = RectilinearGrid(size=(1, 1, 5), x=(0, 10), y=(0, 3), z=[-1, -0.6, -0.5, -0.2, -0.19, 0])
model = NonhydrostaticModel(; grid,
                              closure = ScalarDiffusivity(ν = κₜ, κ = κₜ), 
                              biogeochemistry = LOBSTER(; grid,
                                                          surface_phytosynthetically_active_radiation = PAR⁰,
                                                          carbonates = true,
                                                          advection_schemes = (sPOM = WENO(grid), bPOM = WENO(grid))),
                              advection = nothing)

using BenchmarkTools
@btime column_diffusion_timescale(model)

and I get

with this PR

  199.375 ns (2 allocations: 112 bytes)
0.009900990099009918

on main

  193.690 ns (2 allocations: 112 bytes)
0.009900990099009918

So exactly the same! Thus there was no allocation issue in the first place. I'll revert to more-or-less the version in main for this.

navidcy commented 1 year ago

That the number is the same on main and here it's soothing. Perhaps we should add a test?

navidcy commented 1 year ago

ok, have a look at your convenience :)