+1 from me. Faster is Good. It's a relatively simple fallback if ryu turns out to be unsupportable for some reason. And better to use a well-tested library (presumably?) than our own hacks.
Maybe C++31 will provide more flexible numeric formatting, but while we're waiting for that... On Wed, Jan 6, 2021 at 5:17 PM Paul Ramsey <pram...@cleverelephant.ca> wrote: > Because too many choices is the way to be, I pursued bringing the ryu > library in for numeric output, the same library that PostgreSQL and PostGIS > use, and here's a PR > > https://github.com/libgeos/geos/pull/379 > > Upside is that it's an actual library for this stuff, it's fast, and it's > not a string hack. So if we have any concerns about WKT writing > performance, this is what we should do. However, it's a whole extra library > in our tree, in case we care. > > Thoughts? > > P > > > On Jan 6, 2021, at 3:07 PM, Paul Ramsey <pram...@cleverelephant.ca> > wrote: > > > > OK, so the PR <https://github.com/libgeos/geos/pull/378> now pretty > directly addresses the ticket <https://github.com/libgeos/geos/issues/375> > about the differences in behavior between setTrim(true) and setTrim(false) > which previously switched between a positional and sigfigs oriented output, > and in the PR retains a positional point-of-view throughout. > > > > That still leaves the possibility of using some fancy output library, > like the ones mentioned in https://trac.osgeo.org/geos/ticket/868 or ryu, > which has been vendored into postgis at 3.1. It would certainly be more > elegant than the "strip zeroes" code in the PR. > > > > P > > > >> On Jan 6, 2021, at 12:24 PM, Andrew Hershberger < > andrew.d.hershber...@gmail.com> wrote: > >> > >>> For all these reasons and the fact that the current behaviour has > existed for a long time and is now baked into downstream (those tests in > GeoSwift!!) I'm inclined to just do nothing. > >> > >> Please don't let our tests dissuade you 😅. We'll be more than happy to > adapt to an improved API. > >> > >> On Wed, Jan 6, 2021 at 11:23 AM Paul Ramsey <pram...@cleverelephant.ca> > wrote: > >> > >> > >>> On Jan 6, 2021, at 9:20 AM, Martin Davis <mtncl...@gmail.com> wrote: > >>> > >>> Right, I have now seen that std::setprecision switches to scinot if > the precision is less than the magnitude of the number. Very much not > ideal IMO. So some way of using std::fixed might be needed to solve this. > (Not a problem if decimalPlaces is the default 16 though, I think - numbers > > 10^16 will still be in scinot, but that should be rare). > >>> > >>> So agreed, your PR seems like the right direction. Does it work with > negative numbers and numbers << 1 ? > >> > >> No, on re-reading it's still bogus for small numbers. And negatives. > I'm surprised there's not a standard c++ way to get something like > std::fixed without the trailing zeroes. > >> > >> P > >> > >>> > >>> On Wed, Jan 6, 2021 at 9:01 AM Paul Ramsey <pram...@cleverelephant.ca> > wrote: > >>> For interests sake here's a little program that shows the difference > between std::fixed and the default aka std::defaultfloat. Note that > defaultfloat (which sort of does "what we want" from a trailing zero point > of view), also does sigfigs when restricted to a particular precision and > scientific notation. > >>> > >>> I think if changes are to be made, I like the idea of doing > >>> - default, take the C++ defaults, don't apply anything. this is a > change to current behaviour, which applies a default precision > >>> - if precision is specified, try to do a trimmed, fixed number of > decimals output, which is kind of what my PR does > >>> P. > >>> > >>> > >>> > >>>> On Jan 6, 2021, at 7:54 AM, Martin Davis <mtncl...@gmail.com> wrote: > >>>> > >>>> Well, yes. The current default behaviour seems really unpleasant: > >>>> > >>>> POINT (-0.4225977 46.3406448). ==>. POINT (-0.4225977000000000 > 46.3406447999999997) > >>>> > >>> > >>>> bin/geosop -a "Point (-0.4225977 46.3406448)" -f wkt reducePrecision > 100 > >>>> POINT (-0.4200000000000000 46.3400000000000034) > >>>> > >>>> I agree with Andrew Bell - there is no way GEOS should be trying to > outsmart the C++ language. And add to that, that setting output precision > is a perilous hack, since rounding/truncating data pointwise can result in > invalid topology. > >>>> > >>>> Not saying get rid of the setRoundingPrecision, since it's the user's > decision. But the default should be to just output "full" precision (as > decided by the standard floating-point output routines, which know about > weird things like IEEE-754 guard digits). And forget about trimming, since > the standard output seems to do that just fine. > >>>> > >>>> > >>>> > >>>> > >>>> > >>>> On Wed, Jan 6, 2021 at 7:43 AM Paul Ramsey <pram...@cleverelephant.ca> > wrote: > >>>> For all these reasons and the fact that the current behaviour has > existed for a long time and is now baked into downstream (those tests in > GeoSwift!!) I'm inclined to just do nothing. > >>>> > >>>> Any objections? > >>>> > >>>> P > >>>> > >>>>> On Jan 6, 2021, at 7:41 AM, Andrew Bell <andrew.bell...@gmail.com> > wrote: > >>>>> > >>>>> > >>>>> 1) This fight really can't be won without implementing all the > various things already provided for by a language like C and allowing users > to make these choices for themselves. GDAL, for example, has its own > strange logic to do this kind of thing. It's ugly and it's not obvious to a > user what's going to happen as it's not well-defined by any documentation. > Some users may want the full precision, and spending a bunch of time > figuring out if .999997 is significant or not isn't really the role of a > library like GEOS, IMO. And for some values, scientific notation is what > you need. This is why %g exists for printf in C. > >>>>> > >>>>> 2) If you're using a text file for your output, you really don't > care about size, even if you say you do. Seems like time could be better > spent elsewhere unless someone is paying for this functionality. Someone > could certainly reprocess any WKT file to remove digits if they so chose. > >>>>> > >>>>> On Wed, Jan 6, 2021 at 10:25 AM Martin Davis <mtncl...@gmail.com> > wrote: > >>>>> Is it possible the problem is the use of std:fixed ? (Which is > invoked if the trim option = FALSE, which is the default). > >>>>> > >>>>> Currently in WKTWriter.writeNumber there is this code (and the > defaults invoke fixed precision): > >>>>> > >>>>> if(! trim) { > >>>>> ss << std::fixed; > >>>>> } > >>>>> ss << std::setprecision(decimalPlaces >= 0 ? decimalPlaces : 0) > << d; > >>>>> > >>>>> This results in the following (as noted on the GeoSwift issue) > >>>>> > >>>>> POINT (-0.4225977 46.3406448). ==>. POINT (-0.4225977000000000 > 46.3406447999999997) > >>>>> > >>>>> This carries too much precision, obviously. I think it might be > exposing the IEEE-754 guard digits unnecessarily. FP output is notoriously > tricky, and I suspect it's better to let C++ just do the right thing. > >>>>> > >>>>> Also, running reducePrecision causes problems, again I suspect due > to to imprecise FP representation: > >>>>> > >>>>> bin/geosop -a "Point (-0.4225977 46.3406448)" -f wkt reducePrecision > 100 > >>>>> POINT (-0.4200000000000000 46.3400000000000034) > >>>>> > >>>>> If the std::fixed setting is dropped, the output looks more > reasonable: > >>>>> > >>>>> POINT (-0.4225977 46.3406448). ==>. POINT (-0.4225977234 46.3406448) > >>>>> > >>>>> Check that all input sig digits are shown: > >>>>> > >>>>> POINT (-0.4225977234 46.3406448) ==> POINT (-0.4225977234 46.3406448) > >>>>> > >>>>> Reduced precision displays as expected: > >>>>> bin/geosop -a "Point (-0.4225977 46.3406448)" -f wkt reducePrecision > 100 > >>>>> POINT (-0.42 46.34) > >>>>> > >>>>> > >>>>> Is the "trim" option needed at all? > >>>>> > >>>>> On Tue, Jan 5, 2021 at 3:41 PM Paul Ramsey < > pram...@cleverelephant.ca> wrote: > >>>>> > >>>>> What do people think is the best practice for outputing WKT > precision? > >>>>> _______________________________________________ > >>>>> geos-devel mailing list > >>>>> geos-devel@lists.osgeo.org > >>>>> https://lists.osgeo.org/mailman/listinfo/geos-devel > >>>>> > >>>>> > >>>>> -- > >>>>> Andrew Bell > >>>>> andrew.bell...@gmail.com > >>>>> _______________________________________________ > >>>>> geos-devel mailing list > >>>>> geos-devel@lists.osgeo.org > >>>>> https://lists.osgeo.org/mailman/listinfo/geos-devel > >>>> > >>>> _______________________________________________ > >>>> geos-devel mailing list > >>>> geos-devel@lists.osgeo.org > >>>> https://lists.osgeo.org/mailman/listinfo/geos-devel > >>>> _______________________________________________ > >>>> geos-devel mailing list > >>>> geos-devel@lists.osgeo.org > >>>> https://lists.osgeo.org/mailman/listinfo/geos-devel > >>> > >>> _______________________________________________ > >>> geos-devel mailing list > >>> geos-devel@lists.osgeo.org > >>> https://lists.osgeo.org/mailman/listinfo/geos-devel > >>> _______________________________________________ > >>> geos-devel mailing list > >>> geos-devel@lists.osgeo.org > >>> https://lists.osgeo.org/mailman/listinfo/geos-devel > >> > >> _______________________________________________ > >> geos-devel mailing list > >> geos-devel@lists.osgeo.org > >> https://lists.osgeo.org/mailman/listinfo/geos-devel > >> _______________________________________________ > >> geos-devel mailing list > >> geos-devel@lists.osgeo.org > >> https://lists.osgeo.org/mailman/listinfo/geos-devel > > > > _______________________________________________ > geos-devel mailing list > geos-devel@lists.osgeo.org > https://lists.osgeo.org/mailman/listinfo/geos-devel >
_______________________________________________ geos-devel mailing list geos-devel@lists.osgeo.org https://lists.osgeo.org/mailman/listinfo/geos-devel
