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 wk_trans_explicit() #187

Merged
merged 11 commits into from
Aug 25, 2023
Merged

add wk_trans_explicit() #187

merged 11 commits into from
Aug 25, 2023

Conversation

mdsumner
Copy link
Contributor

@mdsumner mdsumner commented Jul 16, 2023

as advised in #185 this migrates the base part of trans_explicit from crs2crs

  • migrate code from crs2crs
  • create wk_coords()<-
  • make sure use_z use_m are working for replacement function wk_coords
  • fix up doc, e.g. it's not "in-place" exactly
  • outline examples
  • tests
  • unsure how to structure the doc, getting code/doc order mismatch in wk_coords ...
  • worry about leaving objects in a bad state (e.g. not updating the crs)

(it's actually amazing, I think I was confused about what it did in previous times)

@mdsumner
Copy link
Contributor Author

questions/discussion

  • the doc for crs_trans_explicit says "A vector of coordinates as a [wk::xy()] used to replace" but it can actually be any handleable with the same coordinate structure - not a big deal just wondered if I was missing anything becauce the wk_coords<- and wk_trans_explicit doc is now a bit inconsistent

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! This is looking good!

worry about leaving objects in a bad state (e.g. not updating the crs)

I think wk_transform() takes care of this and strips the CRS for anything it returns. You could always add a test of wk_coords(wkt("POINT (0 1)", crs = "EPSG:1234")) <- xy(1, 2) to find out.

it can actually be any handleable with the same coordinate structure

It seems like any non-point replacement will fail at as_xy()?

unsure how to structure the doc, getting code/doc order mismatch in wk_coords ...

I think it's because both wk_trans_explicit() and wk_coords() define value. You could try removing @param value from wk_coords() or if that doesn't work, rename value in wk_trans_explicit()?

R/trans-explicit.R Outdated Show resolved Hide resolved
tests/testthat/test-transform.R Outdated Show resolved Hide resolved
tests/testthat/test-transform.R Show resolved Hide resolved
@mdsumner
Copy link
Contributor Author

mdsumner commented Aug 1, 2023

thanks! I'm getting back to this

mdsumner and others added 2 commits August 1, 2023 10:31
Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>
Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>
@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (7c1f504) 99.03% compared to head (c88225f) 99.03%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master     #187    +/-   ##
========================================
  Coverage   99.03%   99.03%            
========================================
  Files          81       85     +4     
  Lines        5918     6035   +117     
========================================
+ Hits         5861     5977   +116     
- Misses         57       58     +1     
Files Changed Coverage Δ
src/init.c 100.00% <ø> (ø)
R/trans-explicit.R 100.00% <100.00%> (ø)
R/vertex-filter.R 100.00% <100.00%> (ø)
src/trans-explicit.c 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Merge branch 'trans-explicit' of https://github.com/mdsumner/wk into trans-explicit

# Conflicts:
#	tests/testthat/test-transform.R
@mdsumner
Copy link
Contributor Author

mdsumner commented Aug 1, 2023

I got it! as explained in https://adv-r.hadley.nz/functions.html#replacement-functions it has to be function(object, <other arguments>, value)

@mdsumner mdsumner changed the title add wk_trans_explicit() DRAFT (part of #185) add wk_trans_explicit() (part of #185) Aug 1, 2023
@mdsumner mdsumner changed the title add wk_trans_explicit() (part of #185) add wk_trans_explicit() Aug 1, 2023
@paleolimbot
Copy link
Owner

Thank you for taking this on and polishing! I'm on vacation with a 2 and 3 year old and will take a close look + merge when I'm back!

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.

Sorry this took me so long to get to! I did a few minor shuffles but the code looks great!

@paleolimbot paleolimbot merged commit 6330c84 into paleolimbot:master Aug 25, 2023
7 checks passed
@paleolimbot paleolimbot mentioned this pull request Aug 25, 2023
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.

3 participants