🚧 use int for internal coordinate representation and add static function to convert coordinates to int/double and vice versa

[rtroilo]

Changes proposed in this pull request:

improve conversion from long coordinate to double coordinate,

Type of change

• Bug fix (non-breaking change which fixes an issue)

Description

the current implementation leads to a "not so nice" representation of long value coordinate as a double coordinate

 long l = 1231234567L; // internal long for 123.1234567;
 double d = l * OSHDB.GEOM_PRECISION;
 -> 123.12345669999999

This pull request adds static functions to the OSHDB class

• coordinateToLong, coordinateToDouble
  which improve the conversion from long value coordinates to double value and vice versa.

  long l = 1231234567L;
  double convertedDouble = OSHDB.coordinateToDouble(l);
  assertEquals(123.1234567, convertedDouble,0.0);

Checklist

• My code follows the code-style rules and I have checked on the static analyses and benchmark (if applicable) results
• I have commented my code
• I have written javadoc (required for public methods)
• I have added sufficient unit tests
• I have made corresponding changes to the documentation
• I have updated the CHANGELOG.md
• I have adjusted the examples and benchmark if necessary

[tyrasd]

a few general questions:

1. why classify this a bug?
2. does it make sense that these methods live in the OSHDB class? at first glance, it seems like that place looks a bit too prominent?!
3. basically what the implementation does to avoid rounding errors is to use a floating point division instead of a multiplication. note that divisions are not as fast as multiplications on typical CPUs, so this might come with a small(?) performance overhead. I'd guess that for us it wouldn't make a huge difference, but it would perhaps be nice to verify that by some benchmarking.

[rtroilo]

2. does it make sense that these methods live in the OSHDB class? at first glance, it seems like that place looks a bit too prominent?!

According to your question, it makes no sense for you, right? So where do you want to have this methods then?

3. basically what the implementation does to avoid rounding errors is to use a floating point division instead of a multiplication. note that divisions are not as fast as multiplications on typical CPUs, so this might come with a small(?) performance overhead. I'd guess that for us it wouldn't make a huge difference, but it would perhaps be nice to verify that by some benchmarking.

You are right, the multiplication introduces a slightly worse performance. In this case I undo the conversion, to a multiplication over division. The now introduce rounding errors should be handled in outputing methods e.g.

  System.out.printf("%.7f, %.7f", lon, lat);

or for geometries with a precision model

  Point point = ...;
  var pm = new PrecisionModel(10000000);
  var writer = new WKTWriter();
  writer.setPrecisionModel(pm);
  writer.write(point);

[tyrasd]

does it make sense that these methods live in the OSHDB class? at first glance, it seems like that place looks a bit too prominent?!

According to your question, it makes no sense for you, right? So where do you want to have this methods then?

If you ask me: I'd put it somewhere near where they belong conceptionally. And since the coordinates belong (only) to the OSMNodes it might make sense to add it either to the OSMNode class, or perhaps in a new utility class (OSMNodes?).

[rtroilo]

does it make sense that these methods live in the OSHDB class? at first glance, it seems like that place looks a bit too prominent?!

According to your question, it makes no sense for you, right? So where do you want to have this methods then?

If you ask me: I'd put it somewhere near where they belong conceptionally. And since the coordinates belong (only) to the OSMNodes it might make sense to add it either to the OSMNode class, or perhaps in a new utility class (OSMNodes?).

OSHDBBoundingBox also use internally the long coordinate system, and do the long to double conversation.

[tyrasd]

I'd put it somewhere near where they belong conceptionally. And since the coordinates belong (only) to the OSMNodes it might make sense to add it either to the OSMNode class, or perhaps in a new utility class (OSMNodes?).

OSHDBBoundingBox also use internally the long coordinate system, and do the long to double conversation.

True. Then maybe call the utility class Coordinates, EncodedCoordinates or something similar? Do you have any suggestion?

[tyrasd]

I noticed that sometimes we perform Math.round when converting from double to long, and some other times we only cast to (double). Should we be more consistent or does it not matter?

[tyrasd]

• todo: https://github.com/GIScience/oshdb/pull/369/files#r618290231

  So my suggestion here is, to remove the double return value methods completely and rename/change the long return value variants to

  int minLon();
  int minLat();
  int maxLon();
  int maxLat();