-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add cellarea docs, allow Literate.jl tutorials in the doc pipeline, fix typos #800
base: main
Are you sure you want to change the base?
Conversation
amend reference to GDAL
Supersedes #789 |
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.
My main thought here is why doesn't the cellarea
tutorial just go in the cellarea
API docs? (With a link from toolbars)
Or in extended docs? This approach feels like a lot of text needs to be repeated and the overall complexity of the documentation is increased.
To me tutorials should be more about combining multiple methods into workflows.
(Or maybe when one method just has so many options to explain it needs a lot of examples, rasterization may fit this case)
@@ -2,15 +2,16 @@ | |||
""" | |||
reproject(obj; crs) | |||
|
|||
Reproject the lookups of `obj` to a different crs. | |||
Reproject the lookups (axes) of `obj` to a different crs. |
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.
We try to maintain the distinction between axes and lookups as different things
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 see what you're saying, the problem is that even people who've used rasters for a while have no clue what "lookups" are. There has to be some frame of reference. "Dimension values" is also super unclear - what does that mean?
Maybe we can rewrite the parentheses to be more clear? Would "axis values" work there?
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.
FWIW, this is meant to be for people who have no clue what cellarea is or why you might want it. So you have to go from the ground up, more or less.
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 get it, but we just can't use a common base function name that also works on a Raster to explain a DD function that's really a different thing.
It's clearly difficult to find words that describe lookups without semantic overloads (see AxisKeys.jl... keys are a different base method again) but it's important that we try.
I think axis values is helpful, but maybe values is also a pretty empty word. Maybe this needs to be systematic from DD up, we could workshop the language over there.
(FWIW lookup
was intentionally chosen to avoid the overloads with base concepts that other packages have. The problem with avoiding overloads is you end up with a less common word)
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.
Maybe we could say "x and y values" for now?
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.
Then we're back to the problem of "what is a lookup" again?
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.
Maybe we could say "location information". But I would prefer to just use lookups here and have a glossary where we would explain these differences, because otherwise we would have to duplicate this info everywhere we use lookups.
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 would argue that we should duplicate this information everywhere we use lookups, at least in top-level, user accessible functions. I don't want to overestimate the curiosity of a new user v/s their unwillingness to click a link.
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 not to say that we shouldn't have a glossary with a more detailed explanation, but there should be something like:
[lookup values](link to DD lookup docs) (the positions of the cells in each dimension)
or something
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.
Actually "lookup values (indicating the positions of each cell)" sounds pretty descriptive
your cellarea tutorial is not running the lines as intended (no output in most). Plus, is hard to find, I needed to look for it in the search box, maybe add a new section to the nav bar? |
@rafaqz IMHO, API docs should not have extended docs, and be as clean as possible to the main description of input and output arguments. For developers it might be easier to see things there if they work as intended, another test, but documenting should have a low entry level, specially for people (me) that don't know how to use those functions 😄 . |
Sure, I just meant using extended docs so you can add more examples for the online docs that don't swamp the repl. Not so you would actually look for them in the repl. My main point is not to have |
Even extended docs are a little button you can click on online, I don't see an alternative to tutorials unfortunately. These also serve very different uses since the tutorials are to introduce someone to a function and what you can do with it, REPL docs are to remind you of what the function itself does and how to call it |
rasterio also just uses the api docs for functions like this And their tutorials are reserved for general things (though I'm not saying their docs are great) From my perspective, contributors here and for DD have come and gone, and in some years there is a good chance I will have to maintain these docs. Minimizing that task across what are now very many packages is becoming central to maintaining my sanity. If you make tutorials for some Rasters.jl methods, at some point we are committing to making and maintaining them for all methods. I really want to try putting these tutorials in the regular API docs, and changing that when they are really too big and we are committed to fleshing out the docs for every other method here. (possibly some more tutorial style stuff can go in the geocompx book too?) |
Currently we have methods listed here: They could be better organised in a menu somehow. But to me just improving those is better than adding additional versions of them all. Like |
My issue with putting these in docstrings is twofold:
That being said, I can see your point about implicit promises and maintaining stuff. How about this:
|
If the goal of the geocompx books is to replicate the R and Python versions directly then I'm not sure if we can sneak these examples in there, though there will definitely be more examples of these kinds of functions. |
The Rasters API docs already have examples with plots! And they do actually run, as doctests :) I like the split - estimating spatial means is a complete workflow task, perfect for a tutorial It can link to the |
I actually think all of the current API docs can be better and longer with more explanation and examples, I just haven't had time |
|
||
But fundamentally, this is all that `zonal` is doing under the hood - | ||
masking and cropping the raster to the geometry, and then computing the statistic. | ||
=# |
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.
from experience (a painful one), I can tell you that for long explanations using literate and .jl
files is not ideal. I did that for a while in the yax docs, and eventually I went back to just markdown. For small scripts that just run is fine, like in beautifulmakie, but once you start doing bigger things I would say that markdown should be the way to go.
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.
Hmm, why not? It's worked pretty well for me so far, but those were all in situations where my markdown content doesn't exceed ~150 lines. Curious what your issues were...
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 do at some point want to integrate a Quarto like system into most of my docs, since that seems to be the best at this stuff.
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 agree with @lazarusA, Literate.jl adds a layer of complexity that's just not worth it in some cases.
I would prefer not to have it here
# To construct a raster, we'll need to specify the x and y dimensions. These are called "lookups" in Rasters.jl. | ||
using Rasters.Lookups | ||
# We can now construct the x and y lookups. Here we'll use a start-at-one, step-by-five grid. | ||
# Note that we're specifying that the "sampling", i.e., what the coordinates actually mean, |
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.
# Note that we're specifying that the "sampling", i.e., what the coordinates actually mean, | |
# Note that we're specifying the "sampling", i.e., what the coordinates actually mean, |
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 might take this section out, not sure yet.
# Now, let's compute the average precipitation per square meter across Chile. | ||
# First, we need to get the area of each cell in square meters. We'll use the spherical method, since we're working with a geographic coordinate system. This is the default. | ||
areas = cellarea(masked_precip) | ||
masked_areas = mask(areas; with = chile) |
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 do we have to mask again?
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.
#797, but this should be fixed once the cf branch is merged.
@rafaqz now I remember why I was against jldoctest blocks - they don't show up in the REPL at all! So they're fundamentally useless for in-REPL documentation. Unless there's some module level switch to change this (but I don't think there is...) |
Ah I guess we're missing the |
Yeah for me e.g. |
cellarea