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 typos in cell_diffusion_timescale methods. #557

Merged
merged 3 commits into from
Dec 14, 2019

Conversation

ali-ramadhan
Copy link
Member

No description provided.

@ali-ramadhan ali-ramadhan added the bug 🐞 Even a perfect program still has bugs label Dec 7, 2019
@codecov
Copy link

codecov bot commented Dec 7, 2019

Codecov Report

Merging #557 into master will increase coverage by 3.86%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
+ Coverage   67.72%   71.59%   +3.86%     
==========================================
  Files          69       71       +2     
  Lines        1952     2341     +389     
==========================================
+ Hits         1322     1676     +354     
- Misses        630      665      +35
Impacted Files Coverage Δ
...ure_implementations/leith_enstrophy_diffusivity.jl 98.38% <ø> (+66.12%) ⬆️
...rbulenceClosures/turbulence_closure_diagnostics.jl 100% <100%> (+80.76%) ⬆️
src/boundary_conditions.jl 69.06% <0%> (-4.97%) ⬇️
src/halo_regions.jl 83.05% <0%> (-3.07%) ⬇️
src/TimeSteppers/kernels.jl 57.8% <0%> (-2.6%) ⬇️
src/AbstractOperations/computations.jl 75.75% <0%> (-0.25%) ⬇️
src/Solvers/batched_tridiagonal_solver.jl 100% <0%> (ø)
src/logger.jl 0% <0%> (ø)
src/Oceananigans.jl 76.92% <0%> (+1.92%) ⬆️
src/OutputWriters/netcdf_output_writer.jl 89.23% <0%> (+2.69%) ⬆️
... and 9 more

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 a511fdd...129f822. Read the comment docs.

@glwagner
Copy link
Member

glwagner commented Dec 9, 2019

This PR might want to add a simple test that runs these functions to ensure this doesn't continue to happen (and improve coverage), rather than only fixing the current code.

@ali-ramadhan
Copy link
Member Author

@glwagner I added turbulence diagnostics tests to ensure that DiffusiveCFL is compatible with all closures.

Unfortunately it failed on TwoDimensionalLeith as it was bundled with AbstractSmagorinsky and TwoDimensionalLeith does not have a closure.Pr field.

I split it off into its own cell_diffusion_timescale where it's easy to calculate the viscous CFL condition but it seems that TwoDimensionalLeith calculates the nonlinear diffusivity on the fly and so there is no easy/cheap way to compute the diffusive CFL for each tracer.

I'm thinking of merging this PR because I need the bugfix for a script in another repo.

I will open an issue about the diffusive CFL for TwoDimensionalLeith (out of scope for this PR).

@ali-ramadhan ali-ramadhan merged commit 2b2d634 into master Dec 14, 2019
@ali-ramadhan ali-ramadhan deleted the ali-ramadhan-patch-3 branch December 14, 2019 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Even a perfect program still has bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants