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

Update st_ll_sample for regular sample on LINES #725

Merged
merged 5 commits into from
May 29, 2018

Conversation

statnmap
Copy link
Contributor

This update for regular sample is only for lines.

  • Same code as in {sp} for type="regular"
  • Set offset as random for a random starting point.
  • Drop units of l necessary for type="regular", otherwise units problem in grp.
    • It may be necessary to add @importFrom units drop_units
  • If only a unique "size" is fixed, then sample will be regular over all. This means one point every x meter, whatever the length of the line. I guess this is similar to type = random where not the same number of points are sampled on each line.
    • Can maybe add some information about this in the doc and propose to set multiple "size" so that the same number of point is set on each line.

- Same code as in {sp} for `type="regular"`
- Set `offset` as random for a random starting point.
- Drop units of `l` necessary for `type="regular"`, otherwise units problem in `grp`. It may be necessary to add `@importFrom units drop_units`
- If only a unique "size" is fixed, then sample will be regular over all. This means one point every x meter. I guess this is similar to `type = random` where not the same number of points are sampled on each line. Can maybe add some information about this in the doc.
R/sample.R Outdated
message_longlat("st_sample")
st_crs(x) = NA_crs_
}
l = drop_units(st_length(x))
Copy link
Member

Choose a reason for hiding this comment

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

see travis output: drop_units is not found.

R/sample.R Outdated
lcs = c(0, cumsum(l))
grp = split(d, cut(d, lcs, include.lowest = TRUE))
grp = lapply(seq_along(x), function(i) grp[[i]] - lcs[i])
st_sfc(CPL_gdal_linestring_sample(x, grp), crs = st_crs(x))
Copy link
Member

Choose a reason for hiding this comment

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

For PR's in general, try to keep the source formatting, so that we can see what you actually changed.

@statnmap
Copy link
Contributor Author

It was not really easy to find where you call the @importFrom as I usually call them for each function, even if there are repetitions...

  • Whatever, I modified it and run devtools::document to update the Namespace. I do not really know why this changed the order of lines in " R/RcppExports.R" and "src/RcppExports.cpp" but I pushed the changes.
  • I also modified the indentation to be in accordance with the rest of the code

@edzer edzer merged commit a04233a into r-spatial:master May 29, 2018
edzer added a commit that referenced this pull request May 29, 2018
@edzer
Copy link
Member

edzer commented May 29, 2018

Thanks for the PR! Things should now have been merged; please test.

@statnmap
Copy link
Contributor Author

statnmap commented Jun 3, 2018

Tested. It works !
Next step: implement it for polygons as well...

@edzer
Copy link
Member

edzer commented Jun 3, 2018

That one is more tricky! Looking forward to a PR,

@edzer
Copy link
Member

edzer commented Jun 25, 2018

Now available in branch hex.

@statnmap statnmap deleted the statnmap-patch-st_ll_sample branch September 12, 2018 14:13
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.

None yet

2 participants