Submodules for diagnostics and output writers #498
Conversation
|
Can we please merge abstract operations before this. There will be merge conflicts. |
There was a problem hiding this comment.
I love it.
Only comment is that maybe we should remove export statements from Oceananigans associated with diagnostics and output writers.
Then a user can write using Oceananigans.Diagnostics if they need diagnostic stuff, and using Oceananigans.OutputWriters if they need to use output writers. Or they can write
using Oceananigans.OutputWriters: NetCDFOutputWriterI personally like this semantics because 1) I think it's better to give users finer control over what's imported into the namespace and 2) it makes scripts clearer: script-readers know when diagnostics and output writers are used. Etc.
This also requires being a bit more selective about what is exported from the submodules.
|
|
||
| export | ||
| write_output, read_output, | ||
| has_reference, has_array_refs, |
There was a problem hiding this comment.
Should we export has_reference and has_array_refs? Doesn't seem like part of API?
There was a problem hiding this comment.
Probably not. I removed them.
| module Diagnostics | ||
|
|
||
| export | ||
| run_diagnostic, |
There was a problem hiding this comment.
Should we export run_diagnostic?
There was a problem hiding this comment.
Probably not. Removed it.
|
|
||
| using Oceananigans: AbstractDiagnostic | ||
|
|
||
| include("kernels.jl") |
There was a problem hiding this comment.
may want to name this file diagnostics_kernels.jl. We should also probably rename the similarly named file in the nascent TimeSteppers submodule introduced in PR #505 timestepping_kernels.jl.
There was a problem hiding this comment.
I like the idea of more verbose file names. Done!
| using Oceananigans, | ||
| Oceananigans.Operators | ||
|
|
||
| using Oceananigans: AbstractDiagnostic |
There was a problem hiding this comment.
Should AbstractDiagnostic be introduced within this submodule, rather than in Oceananigans?
There was a problem hiding this comment.
Hmmm, I do it in this case because it's used by a shared utility defined in utils.jl. Might make sense to move the shared util into the Diagnostics or OutputWriters module, whichever is defined first so it can be imported by the other.
| Checkpointer, restore_from_checkpoint | ||
|
|
||
| using Oceananigans | ||
| using Oceananigans: AbstractOutputWriter |
There was a problem hiding this comment.
Should AbstractOutputWriter be introduced in this submodule, rather than in Oceananigans? I think concept encapsulation is good.
This PR refactors
diagnostics.jlandoutput_writers.jlinto their own submodules.We might want to discuss #497 as we refactor things into submodules as it increases the number of cross-module import statements.
Helps with #456 and #495