Large numbers formatting errors

[GoogleCodeExporter]

WKTWriterTest tests TestWriteLargeNumbers1 and TestWriteLargeNumbers3 show a 
wrong behavior in NTS.
A double like 123456789012345680 is formatted as "123456789012346000" using a 
fixed precision.
Looks related to .NET formatting specifications, so we need to investigate more.

Original issue reported on code.google.com by diegogu...@gmail.com on 7 Jan 2014 at 2:24

[GoogleCodeExporter]

added specific code that reproduces the bug with r1136

Original comment by diegogu...@gmail.com on 7 Jan 2014 at 2:35

• Changed state: Started

[GoogleCodeExporter]

The test is ok using DoubleConverter class 
(http://www.yoda.arachsys.com/csharp/DoubleConverter.cs), see r1139.
I think that this class should be used only when fixedprecision is requested.

Original comment by diegogu...@gmail.com on 7 Jan 2014 at 3:09

[GoogleCodeExporter]

Having thought about it some more, I think 
- we should leave the code as is 
- reduce the number of digits for the unit test to make it pass, and
- add a notice on why the test values differ.

Formatting doubles is beyond the scope of NTS.

Original comment by felix.ob...@netcologne.de on 8 Jan 2014 at 2:53

[GoogleCodeExporter]

>Formatting doubles is beyond the scope of NTS.
A valid WKT is one of the goals of NTS.
Anyway, the problem is a loss of precision when using a fixed precision format, 
so to me looks acceptable to fix the code or to throw an Exception, but I fear 
to leave the code as is becaus in fact it can generate "invalid" WKT.
Why do you think about using DoubleConverter only when PrecisionModels.Fixed is 
set? 

Original comment by diegogu...@gmail.com on 8 Jan 2014 at 3:10

[GoogleCodeExporter]

> Why do you think about using DoubleConverter only when PrecisionModels.Fixed 
is set?
I think we can keep that in mind, but afaik no one has ever complained about 
this behavior as is. I suppose no one uses coordinates of this magnitude.

Original comment by felix.ob...@netcologne.de on 8 Jan 2014 at 3:14

[GoogleCodeExporter]

>no one has ever complained about this behavior as is
You're right, and, even more, I suppose that no one is interested to format 
with fixed precision such large numbers.

When maxprecisionformat is true a checking is performed to verify if text is 
actually converted without precision loss.
I can also try to extend this stuff also to manage failing tests.

Original comment by diegogu...@gmail.com on 8 Jan 2014 at 3:22

[GoogleCodeExporter]

> 

Actually I wouldn't do that. JTS' writeNumber method looks like this:

  /**
   *  Converts a <code>double</code> to a <code>String</code>, not in scientific
   *  notation.
   *
   *@param  d  the <code>double</code> to convert
   *@return    the <code>double</code> as a <code>String</code>, not in
   *      scientific notation
   */
  private String writeNumber(double d) {
    return formatter.format(d);
  }

Ours is already more complicated.

Original comment by felix.ob...@netcologne.de on 8 Jan 2014 at 3:33

[GoogleCodeExporter]

an acceptable solution (to me): always perform a double check for precision 
loss, that now is performed only when PrecisionModel.Floating is used.
A test like this looks valid to me:
PrecisionModel precisionModel = new PrecisionModel(1E9);
IGeometryFactory geometryFactory = new GeometryFactory(precisionModel, 0);
IPoint point1 = geometryFactory.CreatePoint(new 
Coordinate(123456789012345678000000E9d, 10E9));
Assert.AreEqual(123456789012345690000000000000000d, point1.X);
Assert.AreEqual(10000000000d, point1.Y);
Assert.AreNotEqual("POINT (123456789012345690000000000000000 10000000000)", 
point1.AsText());
Assert.AreEqual("POINT (1.2345678901234569E+32 10000000000)", point1.AsText());

Original comment by diegogu...@gmail.com on 8 Jan 2014 at 3:57

[GoogleCodeExporter]

And what do you want to change in the WKTWriter class?

Original comment by felix.ob...@netcologne.de on 8 Jan 2014 at 4:14

[GoogleCodeExporter]

remove the lines
if (!_useMaxPrecision)
  return standard;
and always perform the check

Original comment by diegogu...@gmail.com on 8 Jan 2014 at 4:16

[GoogleCodeExporter]

nope. my change breaks everything :(

Original comment by diegogu...@gmail.com on 8 Jan 2014 at 4:24

[GoogleCodeExporter]

Don't you think that this will impose a tremendous performance hit?

Original comment by felix.ob...@netcologne.de on 8 Jan 2014 at 4:25

[GoogleCodeExporter]

Absolutely. Probably the only way to fix this stuff is to let the code as is 
and add documentation to the tests.

Original comment by diegogu...@gmail.com on 8 Jan 2014 at 4:35

[GoogleCodeExporter]

added documentation to tests with commit r1141 
problem still here :(

Original comment by diegogu...@gmail.com on 8 Jan 2014 at 4:40

• Changed state: WontFix

[GoogleCodeExporter]

With r1142 I've added a test for DoubleConverter class. 
Looks that the class can be easily usable, 'cause performances are perfectly 
acceptable, if I haven't made mistakes building performance test fixture.

Original comment by diegogu...@gmail.com on 8 Jan 2014 at 8:46

• Changed state: Started

[GoogleCodeExporter]

Read
- http://stackoverflow.com/a/1664351/3175085
- http://csharpindepth.com/Articles/General/FloatingPoint.aspx

You are like Berti Vogts :)
"Nicknamed “Der Terrier” for always fighting for every ball as if it were 
his last"

Original comment by felix.ob...@netcologne.de on 8 Jan 2014 at 9:10

[GoogleCodeExporter]

I'm a fan of "Broken Window Theory" :)
http://www.codinghorror.com/blog/2005/06/the-broken-window-theory.html

Original comment by diegogu...@gmail.com on 9 Jan 2014 at 7:57

[GoogleCodeExporter]

"In order to give numbers that are both friendly to display and 
round-trippable, we parse the number using 15 digits and then determine if it 
round trips to the same value. If it does, we convert that NUMBER to a string, 
otherwise we reparse using 17 digits and display that."
This is perfectly fine, but it isn't what happens in NTS, at least if we use a 
fixed precisionmodel.
So, you're right that we should go over this stuff, but the fact that WKTWriter 
writes wrong results makes me crazy.
In addition, JTS's WKTWrites writes all numbers using ALWAYS no scientific 
notation: using DoubleConverter we can mimic this behavior and remove 
_useMaxPrecision, but I'm not sure if we can introduce some other precision 
errors.

Original comment by diegogu...@gmail.com on 9 Jan 2014 at 8:19

[GoogleCodeExporter]

I think we should try using DoubleConverter.ToExactString(double val) in the 
code. And drop all the other if/try things in the WriteNumber function and see 
if that is ok.

If it is, do you think we can use it or do we have to ask Jon Skeet? Maybe 
there is some hint in his C# In Depth book.

Original comment by felix.ob...@netcologne.de on 9 Jan 2014 at 8:35

[GoogleCodeExporter]

I think the code is from here: http://www.yoda.arachsys.com/csharp/miscutil/

"
Licence

This software is licensed under the Apache licence. The specific licence for 
this library is available here and is also included in the zipped up versions 
of the library. Essentially, you're free to use any part of it in commercial 
software, provided you include a little acknowledgement. It may also, of 
course, be used in open source projects with a compatible licence.
"

BTW I agree with you, if license is ok (and to me looks ok, as far as I know) I 
can try to do this thing soon.

Original comment by diegogu...@gmail.com on 9 Jan 2014 at 8:44

[GoogleCodeExporter]

"you're free to use any part of it in commercial software, provided you include 
a little acknowledgement"
so I think we can simply add the doubleconverter class (and not the entire dll) 
to NTS code with some documentation about how to we have taken this code.

Original comment by diegogu...@gmail.com on 9 Jan 2014 at 8:50

[GoogleCodeExporter]

> so I think we can simply add the doubleconverter class (and not the entire 
dll) to NTS code with some documentation about how to we have taken this code.

Yes, imho NetTopologySuite.Utilities is a good place and perhaps make it 
internal? Maybe not, JSON- or other -Writer classes might profit, too.

Original comment by felix.ob...@netcologne.de on 9 Jan 2014 at 8:59

[GoogleCodeExporter]

DoubleConverter class looks having problems with very large numbers.
123456789012345678000000E9d should be '123456789012345690000000000000000', but 
is formatted as '123456789012345686040493921665024'.
Maybe it's time to close this issue now?

Original comment by diegogu...@gmail.com on 9 Jan 2014 at 11:07

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

So the situation is that:
1. using actual formatting we have some precision loss when no max precision is 
used
2. using DOubleConverter means that no formatting rules are used, so as example 
all decimals are always returned

Given this situation, I think it's right to leave the code as is.

Original comment by diegogu...@gmail.com on 9 Jan 2014 at 12:31

• Changed state: WontFix

[GoogleCodeExporter]

what about Berti "Der Terrier" Vogts? :) https://db.tt/P1NJdzXg

Original comment by diegogu...@gmail.com on 20 Jan 2014 at 2:57