replaced modulo division by % with math.fmod for normalization #275
Conversation
There was a problem hiding this comment.
Thank you! To me this seems fine, but let's see if we can make it even better.
Also it would be great to add new tests for the edge cases (boundary values and zeros, including 0.0 and -0.0). Just extending the test_point_degrees_are_normalized test would be fine, I guess.
geopy/point.py
Outdated
| if modulo < 0: | ||
| latitude = modulo + 90 | ||
| else: | ||
| latitude = modulo - 90 |
There was a problem hiding this comment.
IMO a one-liner would look a bit better there than a duplicated 4-lines condition:
latitude = (modulo - 90) if modulo >= 0 else (modulo + 90)
….0, 0.0, 375) and (-0.0, -0.0, 375)
|
@KostyaEsmukov thanks for reviewing! As for the edge cases, I tried to look at #242 (comment) but longitude = 180.000000000000028 doesn't seem to work no matter fmod or % is used, I think it's because python truncates it even before fmod is called
|
There was a problem hiding this comment.
I tried to look at #242 (comment) but longitude = 180.000000000000028 doesn't seem to work no matter fmod or % is used, I think it's because python truncates it even before fmod is called
The purpose of the edge cases tests is not to test precision, but to specify how values at the boundaries are handled. For example, 180.0 longitude is normalized to -180.0, while someone might expect that 180 is included to the allowed range; while -180.0 is left unchanged. And the same for latitudes with 90-s.
So now the allowed interval for longitudes looks like [-180; 180). And if someone changes modulo >= 0 condition to just modulo > 0, then the interval would become (-180; 180]. It was harder to introduce such a bug before, because there were no ifs, but now, as we have ones, these should be covered by tests in order to avoid regressions.
test/test_point.py
Outdated
| @@ -107,6 +107,10 @@ def test_point_degrees_are_normalized(self): | |||
| self.assertEqual((-85, -175, 375), tuple(point)) | |||
| point = Point(-95, -185, 375) | |||
| self.assertEqual((85, 175, 375), tuple(point)) | |||
| point = Point(-0.0, -0.0, 375) | |||
| self.assertEqual((0.0, 0.0, 375.0), tuple(point)) | |||
There was a problem hiding this comment.
I think we should explicitly check that the negative zeros are normalized to the positive ones, in order to avoid any peculiar bugs (like with atan2).
The issue is that this assertion doesn't verify that, as -0.0 == +0.0:
>>> (-0.0, -0.0, 375) == (0.0, 0.0, 375.0)
True
I think that something like this will do:
point = Point(-0.0, -0.0, 375)
self.assertEqual((0.0, 0.0, 375.0), tuple(point)) # note that the zeros might be negative
# ensure that negative zeros are normalized to the positive ones
self.assertEqual((1.0, 1.0, 1.0), tuple(math.copysign(1.0, x) for x in point))
…ng that negative zeros are normalized to positive ones
|
Thank you very much for your patience. |
Don't worry, we're not in a rush :) Thank you for your efforts.
This is true. But still it would be great to add some tests for this to avoid regressions, even that the behaviour is the same. This might seem to be out of the scope of this task, but as we're here, why not do this at once anyway? :)
Oh, I see, I missed this one out. You're right. That's another reason, I guess, why these boundary values should be covered by tests. There's also a pitfall: though I believe it makes sense to cover these peculiarities with tests. What do you think? BTW the rest of the diff looks good to me. Thanks! |
It's more like I'm bothering you over such a silly things, so thank you anyway!
Wow, I didn't thought this far. |
test/test_point.py
Outdated
| self.assertEqual((-90, -180, 375), tuple(point)) | ||
| point = Point(-270, -540, 375) | ||
| self.assertEqual((-90, -180, 375), tuple(point)) | ||
| initial = (90, 180) |
There was a problem hiding this comment.
Should we also include 90 and 180 in normalization to be consistent ie make abs(latitude) >= 90 and abs(longitude) >= 180?
No, I'd rather not introduce such a change right now without further analyzing. Let's keep it as is for now.
What do you think of a tests, was it too much, or just fine?
Above this line it looks great to me. But the loops below seem to be a bit complex. If Point(-270, -540, 375) or Point(270, 540, 375) test would fail, then anything in the range for counter from 2 to 10 would also fail. And vice-versa: if any of the tests in the range from 2 to 10 would fail, then these two tests should fail too, because there's no such logic in the code which affects behaviour for the values which abs is more than 90/180.
Thereby, IMO it would be enough to simply add a Point(270, 540, 375) test instead of the loops below. And then it should be ready to go 😉
There was a problem hiding this comment.
No, I'd rather not introduce such a change right now without further analyzing. Let's keep it as is for now.
Okay.
Above this line it looks great to me. But the loops below seem to be a bit complex. If Point(-270, -540, 375) or Point(270, 540, 375) test would fail, then anything in the range for counter from 2 to 10 would also fail. And vice-versa: if any of the tests in the range from 2 to 10 would fail, then these two tests should fail too, because there's no such logic in the code which affects behaviour for the values which abs is more than 90/180.
I agree, I just thought that 90 + 180N and 180 + 360N (for N=1..inf) was a prompt to implement it.
Thereby, IMO it would be enough to simply add a Point(270, 540, 375) test instead of the loops below. And then it should be ready to go wink
Added! It was a long run, but definitely not the last ;)
There was a problem hiding this comment.
I just thought that 90 + 180N and 180 + 360N (for N=1..inf) was a prompt to implement it.
Sorry, I wasn't clear enough.
Added! It was a long run, but definitely not the last ;)
Thank you! LGTM, merging. The result is quite good, actually! I'm looking forward to getting more from you 🎉
|
Whoops, the base branch was wrong. It should have been |
replaced modulo division by % with math.fmod
this commit fixes #273