Scope extent in zoom calculations to avoid error

mapbox / mapnik-omnivore

Node module that returns metadata about spatial files.

45 stars 19 forks source link

Scope extent in zoom calculations to avoid error #145

Closed davidtheclark closed 8 years ago

davidtheclark commented 8 years ago

I think this may be connected to https://github.com/mapbox/mapnik-omnivore/issues/142, and may illustrate an incomplete resolution to https://github.com/mapbox/mapnik-omnivore/issues/88.

As far as I can tell, this new fixture is valid GeoJSON, but when we try to get its min/max zoom, we end up hitting the error here: https://github.com/mapbox/mapnik-omnivore/blob/805d115acece01aa1977a0f4ba303bb7977bacf2/lib/utils.js#L27

davidtheclark commented 8 years ago

I added a function that normalizes longitudes by ensuring that they fall within the +/-180 range. This fixes the failing test case and doesn't cause any other tests to fail.

coveralls commented 8 years ago

Coverage increased (+0.04%) to 94.14% when pulling 6aec23d82f82e63df77311efc04c29c1c0f46ed9 on bounds-invalid into 6d548950f069add811b74344da442220896361bc on master.

coveralls commented 8 years ago

Coverage increased (+0.04%) to 94.14% when pulling 6aec23d82f82e63df77311efc04c29c1c0f46ed9 on bounds-invalid into 6d548950f069add811b74344da442220896361bc on master.

coveralls commented 8 years ago

Coverage increased (+0.04%) to 94.14% when pulling 6aec23d82f82e63df77311efc04c29c1c0f46ed9 on bounds-invalid into 6d548950f069add811b74344da442220896361bc on master.

coveralls commented 8 years ago

Coverage increased (+0.04%) to 94.14% when pulling 6aec23d82f82e63df77311efc04c29c1c0f46ed9 on bounds-invalid into 6d548950f069add811b74344da442220896361bc on master.

davidtheclark commented 8 years ago

@rclark Just pinging you so this is on your radar. We talked about it anyway. BTW: Do you think Coveralls has anything to say about it?

rclark commented 8 years ago

💔 coveralls

davidtheclark commented 8 years ago

I suspect that the minzoom and maxzoom numbers I'm getting for the new tests are wrong. I'm not sure, though, and am not sure how to know. So I could use some guidance here from anybody more experienced.

rclark commented 8 years ago

You zoom ranges do not look wrong:

the files are very small, so all the data safely fits in a z0 tile -- minzoom should be 0
the max zoom of 6 is the minimum allowable max zoom, which also fits since your data is not very detailed -- overzooming (looking at z6-simplified data at z >6) will have very little effect

What zoom ranges do you get for these tests without your code change?

davidtheclark commented 8 years ago

Ok, thanks! Without the code changes we get errors thrown instead of zoom ranges.

coveralls commented 8 years ago

Coverage increased (+0.04%) to 94.14% when pulling 3dacc2ea6d9e841e59163ac13a22259b2e122ae6 on bounds-invalid into 6d548950f069add811b74344da442220896361bc on master.

davidtheclark commented 8 years ago

I changed the way the extent gets "normalized" so that it doesn't bother trying to re-scope the longitudes, just stops them at +/-180.

@rclark What do you think about merging this? (Or is there someone else I should ping?) The alternative would be change something upstream so that the end user of an upload gets an error that's more meaningful to them than Error calculating min/max zoom: Bounds invalid.

coveralls commented 8 years ago

Coverage increased (+0.04%) to 94.14% when pulling 54b39cc0e6f6ea35a0247d5efc98dd7aeee7388d on bounds-invalid into 6d548950f069add811b74344da442220896361bc on master.

davidtheclark commented 8 years ago

I'm going to close this and open another, more focused and directed issue/ticket.