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

Add vec_equal support to wk_wkb vectors #183

Merged
merged 18 commits into from Jul 15, 2023

Conversation

anthonynorth
Copy link
Contributor

@anthonynorth anthonynorth commented Jul 9, 2023

Adds vctrs::vec_equal() support for wk_wkb vectors.

Closes #181

@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (e3d4efb) 99.03% compared to head (ca53871) 99.03%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #183   +/-   ##
=======================================
  Coverage   99.03%   99.03%           
=======================================
  Files          81       81           
  Lines        5888     5918   +30     
=======================================
+ Hits         5831     5861   +30     
  Misses         57       57           
Impacted Files Coverage Δ
src/init.c 100.00% <ø> (ø)
R/pkg-vctrs.R 100.00% <100.00%> (ø)
R/wkb.R 93.33% <100.00%> (+0.15%) ⬆️
src/vctr.c 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Owner

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Thank you! All just details 🙂

Is it worth exposing the "convert to hex" in any other way? It is probably useful in other contexts as well...maybe wkb_to_hex() or something?

src/vctr.c Outdated Show resolved Hide resolved
src/vctr.c Outdated Show resolved Hide resolved
src/vctr.c Outdated Show resolved Hide resolved
src/vctr.c Outdated Show resolved Hide resolved
src/vctr.c Outdated Show resolved Hide resolved
tests/testthat/test-wkb.R Show resolved Hide resolved
tests/testthat/test-wkb.R Outdated Show resolved Hide resolved
tests/testthat/test-wkb.R Outdated Show resolved Hide resolved
@anthonynorth
Copy link
Contributor Author

Is it worth exposing the "convert to hex" in any other way? It is probably useful in other contexts as well...maybe wkb_to_hex() or something?

It could be useful. I can add wkb_to_hex to this PR, or a separate PR if you'd like me to do it.

@anthonynorth
Copy link
Contributor Author

Should 2 identical features with different endianness be considered equal? Should vctrs::vec_order(wkb) be endian-dependent (which it currently is)?

# remotes::install_github("paleolimbot/wk#183")
library(wk)

points <- wkt(c("POINT (1 1)", "POINT (2 2)"))
points_le <- wk_handle(points, wkb_writer(endian = 1))
points_be <- wk_handle(points, wkb_writer(endian = 0))

vctrs::vec_equal(points_le, points_be)
#> [1] FALSE FALSE
vctrs::vec_order(points_le) == vctrs::vec_order(points_be)
#> [1] FALSE FALSE

Created on 2023-07-11 with reprex v2.0.2

Copy link
Owner

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Regarding wkb_to_hex()...I'm happy to review that as part of this PR if you'd like to add it and test it (otherwise feel free to omit but open an issue so I don't forget to add it before the next release).

Regarding vec_equal() with different endians...I would personally consider the current behaviour (point_le != point_be) sufficient. If you truly need geometric equality, you will need a geometry engine (e.g., geos_equal())...using == with wkb() or wkt() will always miss some things a user might consider "equal".

src/vctr.c Outdated Show resolved Hide resolved
src/vctr.c Outdated Show resolved Hide resolved
anthonynorth and others added 4 commits July 11, 2023 12:49
Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>
Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>
Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>
Copy link
Owner

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Just one nit! Thank you!

tests/testthat/test-wkb.R Outdated Show resolved Hide resolved
anthonynorth and others added 2 commits July 12, 2023 10:31
Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>
@anthonynorth
Copy link
Contributor Author

I've exported wkb_to_hex() -- but I haven't rebuilt package documentation.

@paleolimbot
Copy link
Owner

Thanks! (FWIW, it's usually good practice to rebuild the documentation yourself)

@paleolimbot paleolimbot merged commit 7c1f504 into paleolimbot:master Jul 15, 2023
7 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.

Add vec_equal support for wk_wkb
3 participants