pyresample
pyresample copied to clipboard
Add initial restructuring of sphinx docs
This PR rewrites the documentation so that it is easier to understand and has more structure. I am generally trying to following the guidelines outlined in this presentation:
https://youtu.be/t4vKPhjcMZg
- [ ] Closes #xxxx
- [ ] Tests added
- [ ] Tests passed
- [ ] Passes
git diff origin/main **/*py | flake8 --diff - [ ] Fully documented
Codecov Report
Merging #434 (b995311) into main (e3d7a2f) will not change coverage. The diff coverage is
n/a.
@@ Coverage Diff @@
## main #434 +/- ##
=======================================
Coverage 94.32% 94.32%
=======================================
Files 79 79
Lines 12905 12905
=======================================
Hits 12172 12172
Misses 733 733
| Flag | Coverage Δ | |
|---|---|---|
| unittests | 94.32% <ø> (ø) |
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
Coverage: 93.843%. Remained the same when pulling b99531113abf0dd23fc566c754f0fef6cd979b9d on djhoese:doc-rewrite-concepts into e3d7a2f306805f2d001d5bb4d546f485e691d80f on pytroll:main.
CC @simonrp84 who said he might be interested in reviewing this after finding the existing documentation hard to deal with.
To anyone reviewing it: there is likely room for improvement but I think the general idea makes sense.
Rendered version of the site: https://pyresample--434.org.readthedocs.build/en/434/
Wow,thanks a lot for that comprehensive rewrite/restructure! Very nice stuff in here!
Some first comments about the concepts.
First, I think the structure is very clear, it's really easy to read through. In some places, I would have liked to have some figures, eg what resampling is in the pyresample context, but I understand that takes maybe more time than you have for this PR.
- API section should be renamed Reference according to the dev guide
- concepts: I think that pyresample can work with arrays of any number of dimensions
- footprint: disc would be more correct than circle.
- Projections: "if you wanted combine..." sentence needs to be reworked a bit.
- Geometries: "typically represent a grid of contiguous pixels that are equally sized." I would rather say "a grid of equally-spaced pixels", ie I think "contiguous" comes with the grid idea, and to stay more generic I would talk about the distance between pixels, rather than their size.
- Swaths: in the description of contiguous data, I think it might worth mentioning eg that the array preserves the topology of the points.
- typo "For data to be consider contiguous" - > considered
- the definition of contiguous is "sharing a common border; touching", I'm not sure this is the right term if we want to be correct, but I have no alternative to propose... yet?
- dynamic areas: "but we don’t know the extents needed to completely contain our swath data when it is resampled" : we shouldn't use "swath" here, it can be confusing, just remove it?
- resampling: "when we transform data from one geometry to another" : i think transform is to vague. Maybe "when we use the data in one geometry to create data points in another geometry"?
- Algorithms: "When resampling we have a lot of options for mapping input pixels to an output pixel." it could be "to output pixels", one input pixels can influence multiple output pixels.
- nearest neighbour: "Nearest neighbor is one of the simplest resampling algorithms available" made me jump :) I understand what you want to convey here, but the algorithm is far from simple... Maybe we should have something along the lines of "nn is one of the simplest ways of mapping input pixels to output pixels"?
- gradient search: "its assumption that pixels are all contiguous" : "its array representation preserves the topology of the observed data"?
- bilinear: maybe mention that it can use a lot of cpu and memory.
- bucket: should be developed a bit more.
I'm stopping here for now, will look more tonight.
Regarding the tutorials, why not have the installation first?
Ok @mraspaud, thanks for the review. That was exactly what I was hoping for. My last couple commits fix everything you've talked about as best I could. There were some things I didn't completely agree with, but not strongly enough to not do it how you said.
The only thing I didn't do is replace all of the use of "contiguous" because I agree we need to have some consistent language/phrase/term for this. "Topologically ordered array"?
Looks good! Had a very quick scan through and a couple of things based on that:
- I think it'd be good to reorganise the "howtos". The current order of pages might be confusing to a new user. Data reduction doesn't seem like a logical starting point. It'd probably be better to have the two
resampling of {xyz} datapages at the top. then maybegeographic definitions, then the projections stuff, then the more advanced things like reduction and preprocessing at the end. - On
Reduction of swath datathere should be a comma in the first sentence, betweenlatsandpyresample. - On
Geometry definitionsthe longitudes range is shown wrongly:[-180;+180[ validity range.. Needs the closing bracket reversed. - I presume the pages about the individual resampler types will be coming back?
Will post more once I've had a proper look through. Thanks for updating it :)
@simonrp84 Good point. I did not do anything with the howtos including their order. It is just a glob pattern and is sorting them alphabetically (I think). I haven't modified the contents of those documents except for small reference or formatting issues. I do mention in the howtos/index page and probably elsewhere that these documents aren't written as they should be and need to be rewritten.
On Geometry definitions the longitudes range is shown wrongly: [-180;+180[ validity range.. Needs the closing bracket reversed.
@simonrp84 I think @mraspaud needs to respond to this. I'm not sure how strict certain parts of pyresample are, but I'm pretty sure Martin and I had a conversation about this exact sentence a long time ago (mostly that I wanted to use []() and he wanted to continue to use [] syntax). I think the idea is that if you do ((lons + 180) % 360) - 180, then you can't have 180 as a final result.
I presume the pages about the individual resampler types will be coming back?
@simonrp84 I shouldn't have deleted any pages. Where was this page you're talking about? Are you thinking of the Satpy documentation? FYI I did add a new page under concepts/resampling that includes information about resampling algorithms, but lacks links to the exact resampler classes.
On Geometry definitions the longitudes range is shown wrongly: [-180;+180[ validity range.. Needs the closing bracket reversed.
@simonrp84 I think @mraspaud needs to respond to this. I'm not sure how strict certain parts of pyresample are, but I'm pretty sure Martin and I had a conversation about this exact sentence a long time ago (mostly that I wanted to use
[]()and he wanted to continue to use[]syntax). I think the idea is that if you do((lons + 180) % 360) - 180, then you can't have 180 as a final result.
I have to admit that I don't remember so much about this. However, if the common usage is to have ]-180, 180], then we should fix pyresample to comply (but obviously not in this PR). To know exactly what is happening now in pyresample, we need to look at the code indeed.
Honestly, I don't care about this wording/correctness as far as this PR goes because it is taking away from the main issue which is we need a term/phrase when referring to arrays that match the topology of the geographic data.
As for the -180/180 thing, the checks in the SwathDefinition were removed in my PR here:
https://github.com/pytroll/pyresample/pull/94/files#diff-631d3c6d4c7d7db5cc47476746ef995875b87005f48dd42f2d2949547de8b598
The checks were as documented and discussed here (+180 triggers a warning). The check_and_wrap utility function created from the original SwathDefinition code retains this behavior. That said, would pyresample work fine in most cases with +180 in the longitudes, yes probably. Let's leave this as is and go on to more important discussions.
@mraspaud @pnuu I rewrote parts of the concepts to better refer to "topology" instead of contiguous pixels. In a couple places I used the phrase "topology preserving" to refer to this property of the geometries/data. I think this PR is ready for re-review and merging.