Skip to content
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

L.Util.formatNum round #6586

Closed
3 tasks done
inkstak opened this issue Apr 1, 2019 · 3 comments · Fixed by #6587
Closed
3 tasks done

L.Util.formatNum round #6586

inkstak opened this issue Apr 1, 2019 · 3 comments · Fixed by #6587

Comments

@inkstak
Copy link
Contributor

inkstak commented Apr 1, 2019

  • I've looked at the documentation to make sure the behavior is documented and expected
  • I'm sure this is a Leaflet code issue, not an issue with my own code nor with the framework I'm using (Cordova, Ionic, Angular, React…)
  • I've searched through the issues to make sure it's not yet reported

Environment

  • Leaflet : 1.3.1 & 1.4.0
  • Browser : Firefox 66 & Chrome 73

To reproduce

L.Util.formatNum(1.005, 2)
> 1
// Returns 1 instead of 1.01

L.Util.formatNum(-1.4837191022531273, 18) 
> -1.4837191022531275
// Returns -1...75 instead of -1...73

The way formatNum performs rounding can result in unexpected changes to coordinates.
For example :

L.GeoJSON.latLngToCoords({  lng: -1.4837191022531273, lat: 43.49222084042808 }, 18)
> [-1.4837191022531275, 43.49222084042808]

The precision argument for latLngToCoords is not documented, but it may be.
I play with french cadastre, and such approximation leads to various mistakes.

Solution

According to this article on stackoverflow, this rounding method should be better :

L.Util.formatNum = formatNum

function formatNum(num, digits) {
  return +(Math.round(num + ('e+' + digits)) + ('e-' + digits))
}
@IvanSanchez
Copy link
Member

I play with french cadastre, and such approximation leads to various mistakes.

That error between -1.4837191022531273 and -1.4837191022531275 is roughly equal to... checks notes... one tenth of an atom. I know that cadastre applications rely on precision, but I don't think one needs that much precision.

In all seriousness now: I acknowledge that it can lead to misdetection of features with the same coordinates.

If you could make that suggestion into a pull request, and see it all the unit tests still pass nicely, I don't think there should be any problem making the change.

@inkstak
Copy link
Contributor Author

inkstak commented Apr 1, 2019

is roughly equal to... checks notes... one tenth of an atom

Sure ! But maths dont lie : Geos & Postgis are really sensible.
Take two polygons that touch, side by side. Sending the GeoJson to a back-end app by changing the 16th decimal, I'll fall into one polygon or another.

parcelles

Pull request in progress.

@johnd0e
Copy link
Collaborator

johnd0e commented Apr 17, 2020

Just for completeness:

#6587 was reverted because of performance issues, but there also was reported more serious flow with numbers in scientific notation:

var line = L.polyline([[0,-7.993322e-10],[10,20],[20,30]]);
L.geoJson(line.toGeoJSON());//errors on NaN
L.Util.formatNum(-7.993322e-10); //Returns NaN

But you may consider "2nd best" approach with epsilon: #7129

With this function we get almost same precision as in your original PR:

L.Util.formatNum(1.005, 2) === 1.01
L.Util.formatNum(-1.005, 2) === -1.01
L.Util.formatNum(1.555, 2) === 1.56
L.Util.formatNum(-1.555, 2) === -1.56
L.Util.formatNum(-7.993322e-10, 16) === -7.993322e-10
L.Util.formatNum(-1.4837191022531273, 18) === -1.4837191022531275 // Round-off error

Performance is great: https://jsperf.com/leaflet-issue-6668/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants