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.

Sign up for GitHub

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

Jump to bottom

updating for start on #291 #309

Merged
merged 3 commits into from
Oct 29, 2016
Merged

updating for start on #291 #309

merged 3 commits into from
Oct 29, 2016

Conversation

jordansread
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 52b6bea on jread-usgs:master into * on USGS-R:master*.

merge conflict
e3ddf53 
Merge branch 'master' of github.com:USGS-R/geoknife

# Conflicts:
#	tests/testthat/test-geoknife_setters.R
@jordansread
Copy link
Contributor Author

jordansread commented Oct 28, 2016 •
edited
Loading

@wdwatkins can you give this a review and try out the new methods for #291 ?

@wdwatkins
Copy link
Contributor

So this is just getting the function ready so other projections can be implemented in the future right? No new functionality yet?

wdwatkins
wdwatkins reviewed Oct 28, 2016
View reviewed changes
Copy link
Contributor

@wdwatkins wdwatkins left a comment

Choose a reason for hiding this comment

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

Everything works for me

R/06-simplegeom-obj.R

stopifnot(grepl('datum=WGS84', proj4string(from)))

simplegeom <- new("simplegeom", from@polygons, proj4string = CRS("+proj=longlat +datum=WGS84"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the datum for this come from the proj4string, so that others can be used in the future? Then other datums can just be added as 'ors' to the grepl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think so.

@wdwatkins
Copy link
Contributor

Travis just failure is just a fluke?

@jordansread
Copy link
Contributor Author

@wdwatkins this does expose a new method, going from an sp::SpatialPolygons object to a simplegeom. You couldn't do that before w/o a hack.

@wdwatkins
Copy link
Contributor

Ok I see, you could only do regular polygons before

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 86.348% when pulling 587956b on jread-usgs:master into 5c109f9 on USGS-R:master.

@jordansread jordansread merged commit ae7dde4 into DOI-USGS:master Oct 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wdwatkins wdwatkins wdwatkins left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jordansread @coveralls @wdwatkins