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

DOCS: cleanup TOC and various docstring format issues #347

Merged
merged 5 commits into from
Sep 19, 2023

Conversation

davemfish
Copy link
Contributor

@davemfish davemfish commented Sep 19, 2023

Fixes #346

I'm interested in feedback & opinions about the table of contents before merging.

Edit: everybody was on board with the TOC change.

I also added some custom css to increase the font-size of the nested TOC items.

@davemfish davemfish marked this pull request as ready for review September 19, 2023 15:24
@davemfish davemfish closed this Sep 19, 2023
@davemfish davemfish reopened this Sep 19, 2023
Copy link
Member

@phargogh phargogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @davemfish ! Overall this looks great, but I was curious about reverting from f-string insertion of the warp algorithms back to statically defining the options.

Comment on lines +13 to +16
build:
os: ubuntu-22.04
tools:
python: "mambaforge-22.9"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love that this takes the build time down from 1265s to 276s

Comment on lines +2211 to +2213
resample_method (string): the resampling technique, one of,
'rms | mode | sum | q1 | near | q3 | average | cubicspline |
bilinear | max | med | min | cubic | lanczos'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you say more about this change?

My thinking behind making this an f-string was that different GDAL versions have different resample methods, and having a way to detect them programmatically and render them to a string for insertion into the docstring meant that we don't need to maintain it quite as much when these options change. Having said that, I don't think I verified that the strings would render in the API docs, so maybe this is the only way to have them display?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I changed it because sphinx was not rendering docstrings for these functions at all. It didn't raise any errors/warnings about it, but pydocstyle couldn't handle it either: PyCQA/pydocstyle#368

I didn't realize that different GDAL versions have different resample methods, though it makes sense. Unfortunately I don't think the variable in the f-string ever gets expanded, whether you're reading the docs on readthedocs, or in the source code, you never get to see the values.

Alternatively, we could just say, "must be one of the options supported by gdal.Warp", or something like that. What do you think? @phargogh

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! That is both neat and highly unexpected. Sphinx must just be flexing/parsing (but not interpreting), and/or using an old version of the language. Either way, good to know!

So, for the current docstring, let's just leave it as-is. I have only ever seen gdal add resample methods (except for the one time they renamed 'nearest' to 'near'), so this is likely to be a subset in future releases.

@phargogh phargogh self-requested a review September 19, 2023 20:39
@phargogh phargogh merged commit 641413d into natcap:main Sep 19, 2023
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API Docs: some missing docs; noisy table of contents
2 participants