-
Notifications
You must be signed in to change notification settings - Fork 297
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
Use lwgeom::st_astext() #800
Conversation
I guess we'll need to wait for r-spatial/lwgeom#25 to reach CRAN before merging. Or is the dev version on github somewhat available from travis and appveyor? |
Re: |
|
Could it be |
How do I trigger that? |
95edae7
to
df917a0
Compare
I squashed all the commits and rebased (I don't know how to pull the edits you made from the pull request). So we have access to the same (buggy) pull request now. When building from 9d093bd with
I get
When running I get this warning
It's a weird coincidence that it searches for |
Sorry @edzer, forget about my last messages I was confused. I see you merged the changes to master without merging the pull request, so I mixed up versions when rebasing the branches. I see what you mean about the build time. It was around 30 minutes before 2166ebf and now is up to 60 minutes. Could it be something with caching? I don't have any issue building master (5a48393) on my machine; it runs in a reasonable amount of time. |
To expand a bit as I'm searching, the check time for
Since travis build is passing, I deduce that it's not using |
I'll work on it during the dev day. @batpigandme, take note! |
@etiennebr I can't add the tag because I don't have access to this repo, but the tag is |
@batpigandme, can you now see the 🤓 label? |
@etiennebr nailed it! |
Closing in favor of #957 |
This accelerates conversion to
WKT
and incidentally makes printing geometries (which usesWKT
quite a lot) much faster. I don't think we can get more effect for such a small effort. I just bind thelwgeom
function to get a significant improvement for very cheap.This PR is conditional on r-spatial/lwgeom#25.
Comparisons
Outputs are similar
Note that
lwgeom::st_astext
spares the spaces between the geometry type and parenthesis (<GEOMETRY> (
) as well as after commas. I find it's an acceptable trade-off and makes the output even more compact.One other thing to mention is that the
precision
argument ofst_as_text()
is not comparable to theprecision
attribute of ansfc
, which can be confusing since they share the same name. Theprecision
argument is in fact the length of the whole output, not only of the decimal portion. This can be disturbing when dealing with coordinates that have very different values inx
andy
, but is no different than the previous behavior. Furthermore, I find that having an explicit and deterministicprecision
argument (that could be named differently) makes it easier to control the output.Much faster
As always, I'm happy to make changes.
Closes #703, #747