-
Notifications
You must be signed in to change notification settings - Fork 191
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
Even better docs #488
Even better docs #488
Conversation
…r ocean convection with plankton example
Codecov Report
@@ Coverage Diff @@
## master #488 +/- ##
==========================================
- Coverage 73.2% 72.73% -0.47%
==========================================
Files 37 37
Lines 1709 1720 +11
==========================================
Hits 1251 1251
- Misses 458 469 +11
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! Still waiting for docs to build on my laptop so I can look at the examples, but looks good to merge. I agree it'll be good to discuss what the docs should look like.
I was hoping to find a citation solution to use in place of all the \citet
and \citep
commands, but don't think I have the time to work on that so maybe manual formatting is the way to go for now?
I think physics.md
contains too many different topics but that can be left to another PR.
Should we refer to the package as Oceananigans.jl
or just Oceananigans
? Oceananigans without backticks might be better as it's not code or a keyword argument, etc.
#"Appendix" => [ | ||
#"Staggered grid" => "manual/staggered_grid.md", | ||
#"Fractional step method" => "manual/fractional_step.md", | ||
#], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep them around as appendices but you were suggesting they're not suitable as appendices in their current form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think they are helpful to users yet. They need a bit of work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's more a matter of opinion.
But my name is not the only one appearing on the docs so I'll work on them until everyone is happy with the appendices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right in general that whether or not docs are more useful than harmful is an opinion... ! But I'm not talking about the content (which you wrote). I'm talking about the way that I chopped up the overleaf and pasted the text in. It's chaotic and random and there's no reference to it in the main part of the docs. It's just noise at this point, the basic work of shaping the content into an appendix and referencing it remains.
src/models.jl
Outdated
function ordered_dict_show(dict, padchar) | ||
if length(dict) == 0 | ||
return string(typeof(dict), " with no entries") | ||
elseif length(dict) == 1 | ||
return string(typeof(dict), " with 1 entry:", '\n', | ||
padchar, " └── ", dict.keys[1], " => ", typeof(dict.vals[1])) | ||
else | ||
return string(typeof(dict), " with $(length(dict)) entries:", '\n', | ||
Tuple(string(padchar, | ||
" ├── ", name, " => ", typeof(dict[name]), '\n') | ||
for name in dict.keys[1:end-1] | ||
)..., | ||
padchar, " └── ", dict.keys[end], " => ", typeof(dict.vals[end]) | ||
) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably belongs in utils.jl
?
Examples starting to look great! We should probably add Few more comments on the examples:
|
docs/src/manual/physics.md
Outdated
the momentum and tracer equations are are solved rotates at a constant rate around a | ||
vertical axis, such that | ||
```math | ||
\bm{f} \equiv f \bm{\hat z} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use =
for the beta-plane definition but \equiv
for the f-plane?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a typo
KaTeX cannot do equation numbers or references yet. There are a number of issues open on katex github about this. I think to suppress output you’d have to add a line with ‘nothing’. But this is silly, so yes, show for the output writers is important. I tried to keep the plotting simple and barebones so 1. we don’t distract from the point of the example, which is to demonstrate Oceananigans usage (not to demonstrate plotting) and 2. so the examples are easy to maintain. I agree that colorbars are useful for interpreting output, but we don’t interpret output in the examples. I think viridis is an acceptable colormap, but I’m also happy to pick a prettier one. As I noted above, I think we should keep plotting simple and involving as few characters as possible so the examples are maximally useful in demonstrating how Oceananigans is used. (There’s really too much code devoted to plotting as is, which we should fix by shipping plotting recipes with Oceananigans.) I thought the package name is Oceananigans.jl? What is convention? Typos noted, I will fix before merging. |
It was a conscious choice on my part (not laziness) to use Note that the quantity of lines is not the only issue; the issue is that one cannot ignore any lines when reading a code, so that each line of code has the potential to distract / confuse and thus should be absolutely meaningful and clear. This issue is especially acute for users who may already be stressed by interpreting code in an unfamiliar language. If we could fence parts of the code off and say "don't read this part because its just for plotting", we could maybe do that, but I think it's strange. Better in my opinion to keep plotting as minimalistic as possible to maximize readability. I really think the value of an example is a strongly diminishing function of its number of lines and its syntactical complexity. I think plotting could be important (or very important) if the plot output is crucial to the value of an example. This could be the case if we want to demonstrate the physics of the code (and I think this could be a good idea). At the moment, our examples do not explain the physics or physical motivation behind the examples. Instead, I designed the examples to be utilitarian to demonstrate valid syntax patterns for setting up a model. The physical aspects of the examples is incidental and not explained or justified (though I do think that for users who understand the physics, this "extra sugar" could be useful --- while causing no harm to users who do not understand the physics). Thus most of the plots simply demonstrate that the code "did something". They don't show anything meaningful in my opinion. I am all for more physically meaningful examples. But we should always think carefully about the purpose of the examples and each line of code they contain. Some of our examples should be about code and not physics --- because this better serves users who do not understand the physics (who would be distracted by lengthy physical justifications / discussions in the examples). If "vorticity" as a title is not clear enough for the two-dimensional turbulence example we can say "Heatmap of vorticity on an arbitrary scale"? Would that be more clear? The point of plotting vorticity is to demonstrate that the "diagnostic" that was defined is actually returning something (any significance of what was returned is just a bonus from the standpoint of that example, not a critical aspect of the example's purpose). Note that when I code things up I often use Note that technically the colormap used for examples is the user's default (not viridis) --- it doesn't show as viridis on my laptop, for example. But I appreciate that viridis is matplotlib default and thus what will appear in the online docs. |
What you've done with the examples makes more sense now if viewed as utilitarian examples meant to show case how you can interact with the code. For users who are trying to familiarize themselves with Julia, maybe it could help if we further split the time stepping and plotting so that all time stepping happens in the first But yeah I'm happy with the examples in this PR. There will be plenty of opportunities to improve. Should we merge this now? The docs have been improved, and would be good to start getting some feedback. |
…s-for-joss Even better docs Former-commit-id: cacb753
This PR rewrites much of the docs, adds a show function for
Model
and various boundary conditions structures (which fixes very long output in the docs), and updates many of the examples to be docs-friendly.The docs are still very much preliminary even after the updates in this PR. Once this PR is merged, I will open an issue to define a roadmap for getting the docs to the place we need them to be.