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

Pre-documentation cleanup #406

Merged
merged 8 commits into from
Sep 15, 2019
Merged

Pre-documentation cleanup #406

merged 8 commits into from
Sep 15, 2019

Conversation

ali-ramadhan
Copy link
Member

Started to write documentation and docstrings but some parts of the code were a little too messy to be documented easily, so I ended up doing some cleanup that seems to deserve its own PR.

I mainly renamed all the abstract types to have the Abstract prefix (following the Julia style guide) and simplified LinearEquationOfState, PlanetaryConstants, and RegularCartesianGrid where I removed properties that weren't being used or that were redundant/useless.

Resolves #190 (because the constants in question have been removed)
Resolves #285

We're going to refactor PlanetaryConstants anyways.
Only left behind a convinient constructor for Earth.
Also removes dim property which is kind of useless as it doesn't tell 
you which dimensions are flat.
@ali-ramadhan ali-ramadhan added the cleanup 🧹 Paying off technical debt label Sep 14, 2019
@ali-ramadhan ali-ramadhan added this to the JOSS milestone Sep 14, 2019
@codecov
Copy link

codecov bot commented Sep 14, 2019

Codecov Report

Merging #406 into master will increase coverage by 1.77%.
The diff coverage is 73.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #406      +/-   ##
==========================================
+ Coverage   71.39%   73.17%   +1.77%     
==========================================
  Files          23       23              
  Lines        1416     1368      -48     
==========================================
- Hits         1011     1001      -10     
+ Misses        405      367      -38
Impacted Files Coverage Δ
...ures/verstappen_anisotropic_minimum_dissipation.jl 52.54% <ø> (ø) ⬆️
...closures/rozema_anisotropic_minimum_dissipation.jl 23.95% <ø> (ø) ⬆️
src/diagnostics.jl 74.19% <ø> (ø) ⬆️
src/time_steppers.jl 74.3% <ø> (ø) ⬆️
src/halo_regions.jl 90.14% <100%> (ø) ⬆️
src/equation_of_state.jl 100% <100%> (ø) ⬆️
src/Oceananigans.jl 100% <100%> (ø) ⬆️
src/turbulence_closures/TurbulenceClosures.jl 56.25% <100%> (ø) ⬆️
src/poisson_solvers.jl 97.65% <100%> (ø) ⬆️
src/utils.jl 64.44% <100%> (ø) ⬆️
... 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 5cd0e33...e5556a7. Read the comment docs.

Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

I approve.

@@ -210,48 +210,12 @@ function start_next_file(model::Model, fw::JLD2OutputWriter)
end
end

####
Copy link
Member

Choose a reason for hiding this comment

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

😏

@ali-ramadhan ali-ramadhan merged commit dd122e9 into master Sep 15, 2019
@ali-ramadhan ali-ramadhan deleted the docstrings branch September 15, 2019 00:14
arcavaliere pushed a commit to arcavaliere/Oceananigans.jl that referenced this pull request Nov 6, 2019
Pre-documentation cleanup

Former-commit-id: dd122e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup 🧹 Paying off technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manual conversions in Grid constructor are not necessary Revise physical constants
2 participants