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

Fix cell_diffusion_timescale with zero tracers #691

Merged
merged 7 commits into from
Mar 11, 2020

Conversation

ali-ramadhan
Copy link
Member

No description provided.

@glwagner
Copy link
Member

glwagner commented Mar 9, 2020

Might be better to use dispatch here (on the case where the kappa-tuple has length zero) for performance, clarity, and julianism!

return min(Δz^2 / closure.νv, Δh^2 / closure.νh,
Δz^2 / max_κv, Δh^2 / max_κh)
if length(closure.κ) == 0
return min(Δz^2 / closure.νv, Δh^2 / closure.νh)
Copy link
Member

Choose a reason for hiding this comment

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

Could define a function cell_viscous_time_scale that returns this

@ali-ramadhan
Copy link
Member Author

I thought of defining cell_viscous_timescale and cell_tracer_diffusion_timescale so that cell_diffusion_timescale just returns

min(cell_viscous_timescale, cell_tracer_diffusion_timescale)

but then cell_tracer_diffusion_timescale would have to return Inf instead of nothing and it seemed a bit messy with lots of one-liner functions and duplicated dispatch signatures so I just refactored cell_diffusion_timescale to just dispatch on whether closure.κ is an empty named tuple or not.

I added tests to make sure cell_diffusion_timescale will work for all closures when buoyancy=nothing, tracers=nothing.

@codecov
Copy link

codecov bot commented Mar 11, 2020

Codecov Report

Merging #691 into master will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #691      +/-   ##
==========================================
+ Coverage   78.05%   78.22%   +0.16%     
==========================================
  Files         120      120              
  Lines        2447     2466      +19     
==========================================
+ Hits         1910     1929      +19     
  Misses        537      537              
Impacted Files Coverage Δ
...rbulenceClosures/turbulence_closure_diagnostics.jl 100.00% <100.00%> (ø)

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 4ad7e93...f6061d6. Read the comment docs.

@ali-ramadhan ali-ramadhan merged commit 7a47b92 into master Mar 11, 2020
@ali-ramadhan ali-ramadhan deleted the ar/fix-closure-diagnostics branch March 11, 2020 13:43
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