-
Notifications
You must be signed in to change notification settings - Fork 265
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 BCMath (where available) for all float arithmetic #115
base: master
Are you sure you want to change the base?
Conversation
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 |
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. |
Just a small tracklog from the tests/input folder :) |
…, centroid(), and pointInPolygon().
…gth(), greatCircleLength(), haversineLength(), lineSegmentIntersect().
…es a lot of decimal places (ie: latitude/longitude).
… that uses a lot of decimal places (ie: latitude/longitude)." This reverts commit 1298694.
Rebased onto latest master, fixed a bug, and removed the This PR is still not ready to be merged, however.
|
@BathoryPeter I wonder if this will be more reasonable without the |
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. |
Currently 2 tests are failing:
Both have to do with calculated lengths of |
@mstenta I wonder if we should open this PR against the new fork of this repo: https://github.com/itamair/geoPHP |
@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! |
See the original issue that spawned this branch: #114