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

Submodules for grids, solvers, diagnostics, and output writers #512

Merged
merged 13 commits into from
Oct 30, 2019

Conversation

ali-ramadhan
Copy link
Member

This PR refactors each of grids.jl, poisson_solvers.jl, diagnostics.jl and output_writers.jl into their own submodules.

Moving stuff into modules is a pretty disruptive change that introduces tough merge conflicts into other PRs so I also created a Grids and a Solvers submodule in preparation for the vertically stretched grid as I didn't want to include them with vertically stretched PRs that could sit open for days.

The whole code still feels a bit messy to me but I think working on #497 will help a lot.

Helps with #456 and #495

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.

It's hard to wrap my head around this but scanning the new source structure, it looks like a move in the right direction.

@glwagner
Copy link
Member

Looks like there are some issues with examples, and that's why the doc builds failed.

@glwagner
Copy link
Member

As a side note: I think it's ok to include "shared utils" at the top level within a submodule prior to importing logically-distinct functionality contained in separate files. Sometimes this can improve code-readability if there are a small number of utils. The need for a separate "utils.jl" file should decrease when the code structure is more modular and separated into logical subunits, I think.

We can also have single-file submodules, as in Documenter.jl (I think their file structure looks very sane and manageable):

https://github.com/JuliaDocs/Documenter.jl/tree/master/src

There's still a bit of work we need to do to understand inter-submodule dependencies; once that's sorted out I think the top-level Oceananigans.jl file will clean up.

@glwagner
Copy link
Member

glwagner commented Oct 29, 2019

By the way, with a Grids submodule we can rename RegularCartesianGrid to Grids.RegularCartesian (and Grids.VerticallyStretchedCartesian). This may help with script readability and script-writing. We may even eventually have a subsubmodule Grids.Cartesian.Regular; Grids.Cartesian.VerticallyStretched.

@codecov
Copy link

codecov bot commented Oct 30, 2019

Codecov Report

Merging #512 into master will decrease coverage by 2.01%.
The diff coverage is 68.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #512      +/-   ##
==========================================
- Coverage   67.68%   65.66%   -2.02%     
==========================================
  Files          49       65      +16     
  Lines        1838     1896      +58     
==========================================
+ Hits         1244     1245       +1     
- Misses        594      651      +57
Impacted Files Coverage Δ
src/Operators/ops_regular_cartesian_grid.jl 84.21% <ø> (ø) ⬆️
src/AbstractOperations/AbstractOperations.jl 33.33% <ø> (ø) ⬆️
src/Oceananigans.jl 75% <ø> (ø) ⬆️
src/TurbulenceClosures/TurbulenceClosures.jl 100% <ø> (ø) ⬆️
src/Solvers/poisson_solver_gpu.jl 0% <0%> (ø)
src/Grids/Grids.jl 100% <100%> (ø)
src/Solvers/poisson_solver_cpu.jl 100% <100%> (ø)
src/Diagnostics/Diagnostics.jl 100% <100%> (ø)
src/TimeSteppers/TimeSteppers.jl 74.35% <100%> (ø) ⬆️
src/Solvers/Solvers.jl 100% <100%> (ø)
... and 36 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 4b8ff03...9060a5f. Read the comment docs.

@ali-ramadhan
Copy link
Member Author

ali-ramadhan commented Oct 30, 2019

As a side note: I think it's ok to include "shared utils" at the top level within a submodule prior to importing logically-distinct functionality contained in separate files. Sometimes this can improve code-readability if there are a small number of utils. The need for a separate "utils.jl" file should decrease when the code structure is more modular and separated into logical subunits, I think.

Hmmm, I like having separate utils.jl files because it gives me a default place to put utility functions and look for them. I also like keeping the top-level module file strictly for import/export statements and include statements.

Been thinking that the main Oceananigans.jl module file could be cleaned up quite a bit, especially if it becomes just import/export/include statements.

At some point we should discuss how to structure the package code then everything should become much cleaner.

@ali-ramadhan ali-ramadhan merged commit c3c1f13 into master Oct 30, 2019
@ali-ramadhan ali-ramadhan deleted the ar/four-submodules branch October 30, 2019 22:52
arcavaliere pushed a commit to arcavaliere/Oceananigans.jl that referenced this pull request Nov 6, 2019
…#512)

Submodules for grids, solvers, diagnostics, and output writers

Former-commit-id: c3c1f13
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.

2 participants