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(geospatial): add examples for duckdb supported methods #8128

Merged
merged 1 commit into from Jan 31, 2024

Conversation

ncclementi
Copy link
Contributor

@ncclementi ncclementi commented Jan 29, 2024

Description of changes

  • Add docstring examples for duckdb geospatial functions

Issues closed

Closes #7959

TODO/Problems/need help

  • Understand why CI is broken and fix it.
  • Make sure examples render properly in CI, I can't get the docs to build locally because of seg fault (xref BUG: Seg Fault when st_aswkb() to arrow duckdb/duckdb_spatial#236)
  • Need to figure out quarto syntax to include in the geospatial expressions page point (which is in numeric types)
  • Decided what to do about a few methdos that are specific to LineStrings and the zones example doesn't contain such type of geometry.

@ncclementi ncclementi self-assigned this Jan 29, 2024
@ncclementi ncclementi added docs Documentation related issues or PRs geospatial Geospatial related functionality docs-preview Add this label to trigger a docs preview labels Jan 29, 2024
@ibis-docs-bot ibis-docs-bot bot removed the docs-preview Add this label to trigger a docs preview label Jan 29, 2024
Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

Thanks for churning through this!

Can you also apply ruff format .? The version of ruff listed in pyproject.toml should automatically format the code in docstrings.

ibis/expr/types/geospatial.py Outdated Show resolved Hide resolved
ibis/expr/types/geospatial.py Outdated Show resolved Hide resolved
ibis/expr/types/geospatial.py Outdated Show resolved Hide resolved
ibis/expr/types/geospatial.py Outdated Show resolved Hide resolved
ibis/expr/types/geospatial.py Outdated Show resolved Hide resolved
ibis/expr/types/geospatial.py Show resolved Hide resolved
ibis/expr/types/geospatial.py Outdated Show resolved Hide resolved
ibis/expr/types/geospatial.py Outdated Show resolved Hide resolved
ibis/expr/types/geospatial.py Outdated Show resolved Hide resolved
ibis/expr/types/geospatial.py Outdated Show resolved Hide resolved
@ncclementi
Copy link
Contributor Author

The failures I'm seeing are weird, because the expected and the result match. This is only happening for the operations that return geometries. The only difference I see is that the result seems to be truncated shorter than the expected value. But I'm not sure if that can be the problem.

@gforsyth
Copy link
Member

The failures I'm seeing are weird, because the expected and the result match. This is only happening for the operations that return geometries. The only difference I see is that the result seems to be truncated shorter than the expected value. But I'm not sure if that can be the problem.

It's a character-by-character check, so having the results displayed in fewer columns will break the doctests

@ncclementi
Copy link
Contributor Author

It's a character-by-character check, so having the results displayed in fewer columns will break the doctests

@gforsyth ok, this makes sense, but I'm using whatever is the default but what gets produced on my end but that doesn't seem to match whatever CI is producing.

@gforsyth
Copy link
Member

@gforsyth ok, this makes sense, but I'm using whatever is the default but what gets produced on my end but that doesn't seem to match whatever CI is producing.

yeah, it's frustrating. The output gets resized depending on terminal width, so I would recommend narrowing your terminal (and then making note of how many columns you want the output to take up)

@gforsyth
Copy link
Member

yeah, it's frustrating. The output gets resized depending on terminal width, so I would recommend narrowing your terminal (and then making note of how many columns you want the output to take up)

The github actions terminal is set to an 80 character width. Might be possible to change that, but for now, I would just set your terminal to that for generating the doctest outputs

@ncclementi
Copy link
Contributor Author

Ok getting closer.
The ones failing, I'm expecting them to fail (kind off) because of #8143 but, somehow that is working on CI, though the output is different because I was not able to create the expected output locally.

@ncclementi ncclementi added the docs-preview Add this label to trigger a docs preview label Jan 30, 2024
@ibis-docs-bot ibis-docs-bot bot removed the docs-preview Add this label to trigger a docs preview label Jan 30, 2024
@ibis-docs-bot
Copy link

ibis-docs-bot bot commented Jan 30, 2024

@ncclementi
Copy link
Contributor Author

ncclementi commented Jan 30, 2024

I think this is ready for a final look, got to render the NumericValue.point in the geospatial expressions page too. I think it looks good.

EDIT: Not so fast!

@ncclementi ncclementi added the docs-preview Add this label to trigger a docs preview label Jan 31, 2024
@ibis-docs-bot ibis-docs-bot bot removed the docs-preview Add this label to trigger a docs preview label Jan 31, 2024
@ibis-docs-bot
Copy link

ibis-docs-bot bot commented Jan 31, 2024

Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

SWEET 🚢 it!

@ncclementi
Copy link
Contributor Author

This is ready for final review.

We'll solve/discuss the literals issue #8143 in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related issues or PRs geospatial Geospatial related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: add example docstrings to geospatial methods
3 participants