Turfjs / turf

A modular geospatial engine written in JavaScript and TypeScript
https://turfjs.org/
MIT License
9.4k stars 944 forks source link

Random isn't random for small values #218

Closed mattkrick closed 9 years ago

mattkrick commented 9 years ago

The following code produces this result:

    bboxCoords = [144.8613, -37.9136, 145.0613, -37.7136];
    poly = turf.bboxPolygon(bboxCoords);
    points = turf.random('points', 10, {bbox: bboxCoords});

image

Seems to work fine when the bbox is big enough. PS would also be nice to either throw an error or swap coords if low > high, as this still draws a bbox correctly, but assigns random points incorrectly.

tmcw commented 9 years ago

Gotcha. Will look into this tomorrow, repo is at https://github.com/mapbox/geojson-random/blob/master/index.js#L70-L77 - looks like my usage of the % isn't right.

mattkrick commented 9 years ago

I'd suggest the following (felt too short to make a PR). Pseudo-RNGs aren't very expensive & I'd wager that removing the conditionals would cancel out the cost of the extra RN, assuming your use of only 1 RN was for extreme efficiency reasons. If that's the case, I'd also suggest passing in pre-calculated latSpan & lonSpan in module.exports.point since it's just looping the exact same calculation. Cheers & a serious thank you for turf, it's everything I've been hoping for.

  function coordInBBBOX(bbox) {
    var lonSpan = bbox[2] - bbox[0],
      latSpan = bbox[3] - bbox[1],
      randLon = Math.random() * lonSpan,
      randLat = Math.random() * latSpan;
    return [bbox[0] + randLon, bbox[1] + randLat];
  }
tmcw commented 9 years ago

The use of 1 RN wasn't for efficiency reasons: it was because if you have a rectangle with a high aspect ratio and you take the 2-random-numbers approach, you're just as likely to pick points along the long axis as the short - so they aren't evenly distributed in space.

mattkrick commented 9 years ago

Ahhh, that's interesting! Although I have to admit I don't understand. Is there some reading you could point me towards? Might be a good mapbox blog post! Using 2 RNs on a high aspect ratio bbox looks random (see below), but I'll be the first to admit I'm no GIS expert. image

tmcw commented 9 years ago

Hm, maybe my thesis was totally wrong :)

tmcw commented 9 years ago

:+1: yep it was :cry: https://github.com/mapbox/geojson-random/commit/cd1b75a38b78d322d34835a42bce792b6e3dfff9 https://github.com/Turfjs/turf-random/commit/590bd57c86bfeb13aafd1677967b47d0d3d088fe - fixed in 1.0.2 of turf-random and will roll out in next turf version.