-
Notifications
You must be signed in to change notification settings - Fork 2
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
Improve WKB performance. #39
base: main
Are you sure you want to change the base?
Conversation
At some point we will need wkb geometry types that store ncoord/hasm/hasz in the type for wkb to be fast (we could just put them in GI wrappers ) Using Val at the getcoord level isn't so much of a performance gain as we are still lifting runtime values to types in inner loops. Ultimately you want type stability at the level of a whole polygon, or better for a vector of polygons. Also in terms of GeometryOps or Rasters performance the key is defining |
WKB is a storage type, not a good in memory type, that's why I never made it a struct on its own, just a good enough parser/serializer. Feel free to wrap them, or even convert them to somewhere else. Agreed that a single, topmost I will add manual xyzm methods here. |
@@ -219,7 +224,7 @@ function GI.getgeom( | |||
geom::WKBtype, | |||
i::Integer, | |||
) | |||
ncoord = GI.ncoord(geom) | |||
ncoord = GI.ncoord(T, geom) | |||
GI.getgeom(T, geom, i, ncoord) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an internal method rather than adding non-standard methods to GI.getgeom
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, not part of this PR, but I should fix that yes. It seems I already thought of your suggestion to pass ncoord form the toplevel in a hacky way.
Adding specialised |
Mostly I'm just thinking about minimising read time |
Fixes #32. Points are now Tuples, so this will be a breaking release (will release a minor release first for the other PRs).
NTuple is type unstable if ncoord is not known, so let's make it
Val
. Shortcutting toGI.ncoord(T, geom)
also helps a lot of unnecessarygeomtrait
calls that are already known. Writing out geomtrait also helped.