Use BCMath (where available) for all float arithmetic

[mstenta]

See the original issue that spawned this branch: #114

[BathoryPeter]

I measured length() function with BCMath enabled and disabled and the difference is more than significant.

$geom = GeoPHP::load(file_get_contents('20120702.gpx'));
$start = microtime(true);
for($i=0; $i < 50; $i++) {
    $geom->length();
}
var_dump(microtime(true) - $start);

BCMath enabled: 21.463 sec
BCMath disabled: 0.579 sec

[mstenta]

Whoa, 21 seconds is a long time! What's in that file?

BCMath is going to be more intensive, yes. But it is also going to be more precise. For some that might matter more than speed. Perhaps we should include some ability to globally enable/disable bcmath usage within GeoPHP?

Also, there are some issues with LineString::greatCircleLength() and LineString::haversineLength() methods: #114 (comment)

I posted that to the other issue, but forgot to update this pull request. This should be put on hold until that's fixed.

[BathoryPeter]

Just a small tracklog from the tests/input folder :)

[mstenta]

Rebased onto latest master, fixed a bug, and removed the bcscale(24) code. See #114 (comment) for more details.

This PR is still not ready to be merged, however.

Also, there are some issues with LineString::greatCircleLength() and LineString::haversineLength() methods: #114 (comment)

[mstenta]

I measured length() function with BCMath enabled and disabled and the difference is more than significant.

@BathoryPeter I wonder if this will be more reasonable without the bcscale(24) code. I assume BCMath operations will still be more intensive than regular PHP arithmetic, so it will still be slower, but maybe better with default bcscale() precision.

[mstenta]

FYI: I kept my commits separate to show the reasoning behind these recent changes, but we can plan to squash them into a single commit when it's time to merge.

[mstenta]

Currently 2 tests are failing:

There were 2 failures:

1) Tests_20120702::testMethods
Failed on length
Failed asserting that 0.11624637315233 matches expected 0.

/home/travis/build/phayes/geoPHP/tests/tests/20120702Test.php:217
/home/travis/build/phayes/geoPHP/tests/tests/20120702Test.php:47

2) MethodsTests::testMethods
Failed on length (test file: 20120702.gpx)
Failed asserting that 0 is not equal to 0.

/home/travis/build/phayes/geoPHP/tests/tests/methodsTest.php:218
/home/travis/build/phayes/geoPHP/tests/tests/methodsTest.php:52

Both have to do with calculated lengths of MultiLineString geometries. It makes sense that this would be affected by these changes, because using BCMath will result in more precise calculations. Maybe we just want to update the tests to look for these more precise values instead? We may also need to set bcscale() in the tests to specify the precision to use.

[paul121]

@mstenta I wonder if we should open this PR against the new fork of this repo: https://github.com/itamair/geoPHP

[mstenta]

@paul121 Yea probably. Although maybe we should fix the failing tests first, and review it all again. It's been a long time since I've looked at this, although we've been using it in farmOS for almost 10 years now!

[peacog]

Hi @mstenta @paul121
We've found a bug with the bcmath functions. Bcmath numbers are expected to be strings, not floats. Sometimes casting a float to a string gives you scientific notation. For example 0.00004834373416429116 gets converted to 4.8343734164291E-5, and a bcmul(): Argument #2 ($num2) is not well-formed error is thrown. To fix this we need to explicitly convert the float to string before passing it to bcmul().

I've prepared a patch but don't have permission to push it here. Could you update this PR with my changes? I've attached a patch file here. Note the patch only covers the functions that fail for us. There may well be other places where the float should be converted to string before being passed to a bcmath function.

Thanks!

geophp-bcmath-string.patch

[mstenta]

Thanks for reporting that @peacog! I see what you mean about the potential issue.

However, I think the patch would need some more work before we can consider merging it into this PR.

1. Hard-coding $precision = 12; is problematic. In farmOS, for instance, we set bcscale(24) to ensure higher precision, so this would reduce the precision. But see my comments above about why we can't make assumptions like that in this library.
2. The use of number_format() in the current patch will break the library when numbers over 1000 are used, because it is going to add the default , thousands separator. We could explicitly set that parameter to "" in the function calls, but I wonder if there's a better alternative to number_format(). 🤔
3. We should review all the places where it might be necessary, so that this PR is "feature complete".

I think we'll need to do more research on the proper way to prevent the issue you encountered.

[peacog]

Thanks @mstenta. I was looking for a quick way to patch the problem on our site, hence the partial patch.

1. I see here that you were going to set precision in farmOS. Did you do that in the end?
2. We could use number_format($number, $precision, '.', '') to remove the thousands separator
3. I agree. I do want a quick way to patch our installation though, pending this 🙂. Can you recommend a way for me to do it? I tried adding my own composer patch to the drupal root composer.json but couldn't get it to work correctly with the farmOS composer.json

[peacog]

@mstenta as I said in my comment here this error might have been introduced with PHP 8.

We could solve the precision problem by using this float_to_string function instead of number_format

private function float_to_string($float) {
  $string = (string)$float;

  if (preg_match('~\.(\d+)E([+-])?(\d+)~', $string, $matches)) {
    $decimals = $matches[2] === '-' ? strlen($matches[1]) + $matches[3] : 0;
    $string = number_format($float, $decimals,'.','');
  }

  return $string;
}