-
Notifications
You must be signed in to change notification settings - Fork 25
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 ryu for faster float-to-string conversion #118
Comments
I think that sounds really promising. I’d prefer not to lose support for integer geometries though. I wonder if there’s a way to accommodate both?
|
This was discussed a little in rust-lang/rust#52811. |
I'm not exactly sure how that would work because the existing implementation is using a blanket trait implementation. So if I try to implement something also on impl<T> fmt::Display for Coord<T>
where
T: WktNum + fmt::Display,
{
fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
write!(f, "{} {}", self.x, self.y)?;
if let Some(z) = self.z {
write!(f, " {}", z)?;
}
if let Some(m) = self.m {
write!(f, " {}", m)?;
}
Ok(())
}
}
impl<T> fmt::Display for Coord<T>
where
T: WktNum + ryu::Float + fmt::Display,
{
fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
write!(f, "{} {}", self.x, self.y)?;
if let Some(z) = self.z {
write!(f, " {}", z)?;
}
if let Some(m) = self.m {
write!(f, " {}", m)?;
}
Ok(())
}
} it errors with:
Is there some way to use a single implementation and tell whether that |
I don't think we can solve it as phrased. But maybe we could achieve what we want (fast float WKT while still allowing integer WKT) doing something like we did in geo for our integer vs. float Kernel implementations: |
That would essentially remove the blanket implementation and implement on the raw number types instead? |
Right - I think that's a fair trade. I have yet to encounter someone actually using any of our geo libraries for an exotic numeric type, but I'd be interested in seeing how hard it is to add support for them (or to give them the tools to add support for themselves) if we happen to break things for them. |
To be clear, I'm suggesting we could break things for exotic numeric types and wait to see if anybody cares before we put a bunch of effort into supporting it. |
In geoarrow/geoarrow-rs#788 @b4l prototyped a custom implementation of writing to WKT, and he said
It doesn't look like this
wkt
crate usesryu
, and so I'm curious if that's a large part of why his implementation is faster. https://github.com/dtolnay/ryu seems to be a very popular library, and so it doesn't seem like an unreasonable dependency.It looks like it should be relatively easy to swap in
ryu
and test if that's significantly faster? It looks likewkt/src/types/coord.rs
Lines 37 to 44 in c3088cd
I suppose the primary issue with using ryu is that ryu's
Float
trait supports onlyf32
andf64
, whileWktNum
supports a broader range of types, including integers.Assuming that ryu is indeed quite a bit faster, I think it's worth special casing
f32
andf64
if we can, assuming that most real world data will bef64
.(For maintainability reasons, if geoarrow-rs can use
wkt
directly, that's much more appealing to me than rolling our own).Any thoughts?
The text was updated successfully, but these errors were encountered: