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

Fix for rounding issue when formatting whole number #78

Merged
merged 1 commit into from
Oct 11, 2016
Merged

Fix for rounding issue when formatting whole number #78

merged 1 commit into from
Oct 11, 2016

Conversation

0x686578
Copy link
Contributor

@0x686578 0x686578 commented Oct 9, 2016

The function sprintf discards digits after the decimal point if the format string '%d' is used, causing numbers like 35.999999999999 to be displayed as 35 instead of rounding it to 36.

This results in some quirks with coordinates like LatLongValue(52.01, 10.01) with a precision of 0.01 (in DMS format), which would be displayed as

52° 0' 35" N, 10° 0' 35" E

but should be properly rounded and displayed as

52° 0' 36" N, 10° 0' 36" E

Another example is LatLongValue(52.02, 10.02) with a precision of 0.01, which would be displayed as

52° 1' 12" N, 10° 1' 11" E

but should be displayed as

52° 1' 12" N, 10° 1' 12" E

This patch fixes this rounding issue by simply using the format string '%.0F' (0 digits after the decimal point) for whole numbers instead, in which case the number is properly rounded.

@JeroenDeDauw
Copy link
Member

JeroenDeDauw commented Oct 9, 2016

Thanks for the pull request!

+1 from my side but I'd like someone with more attention to detail to check this.

@mariushoch
Copy link
Member

+1. Change looks good to me, but I'm not into this enough to confidently merge it.

@thiemowmde
Copy link
Contributor

thiemowmde commented Oct 11, 2016

This is a rare IEEE edge cases, when rounding leaves a number that's a billionths of a degree smaller, but %d simply cuts instead of rounding up. I tested this for a while and can approve that this is a correct fix. Thanks a lot for submitting it!

@thiemowmde thiemowmde merged commit afd6523 into DataValues:master Oct 11, 2016
@thiemowmde thiemowmde added this to the 1.1.8 milestone Oct 11, 2016
@0x686578
Copy link
Contributor Author

You're welcome and thanks for merging. It's weird though that this wasn't noticed before with the unit test that I also needed to correct.

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

Successfully merging this pull request may close these issues.

None yet

4 participants