replaced modulo division by % with math.fmod for normalization

[paulefoe]

replaced modulo division by % with math.fmod
this commit fixes #273

[KostyaEsmukov]

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.

[KostyaEsmukov]

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)

[paulefoe]

@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

180.000000000000028 + 180
360.0

[KostyaEsmukov]

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.

[KostyaEsmukov]

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))

[paulefoe]

Thank you very much for your patience.
As for normalization nothing changed compared to original version because absolute value should be bigger than 90 and 180 for latitude and longitude respectively to be normalized. This means that 180 is not going to be normalized to -180 and same to 90s.
Allowed intervals for longitudes are [180, -180] and same for latitudes with 90s

[KostyaEsmukov]

Thank you very much for your patience.

Don't worry, we're not in a rush :) Thank you for your efforts.

As for normalization nothing changed compared to original version

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? :)

because absolute value should be bigger than 90 and 180 for latitude and longitude respectively to be normalized. This means that 180 is not going to be normalized to -180 and same to 90s.

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 90 and 180 are excluded from normalization, 90 + 180*N and 180 + 360*N (for N=1..inf) are not. For example, -540 longitude would be normalized to -180.0 (which is expected), but 540 would also become -180.0 instead of a probably expected positive 180.0.

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!

[paulefoe]

Don't worry, we're not in a rush :) Thank you for your efforts.

It's more like I'm bothering you over such a silly things, so thank you anyway!

There's also a pitfall: though 90 and 180 are excluded from normalization, 90 + 180N and 180 + 360N (for N=1..inf) are not. For example, -540 longitude would be normalized to -180.0 (which is expected), but 540 would also become -180.0 instead of a probably expected positive 180.0.

Wow, I didn't thought this far.
Should we also include 90 and 180 in normalization to be consistent ie make abs(latitude) >= 90 and abs(longitude) >= 180?
What do you think of a tests, was it too much, or just fine?

[KostyaEsmukov]

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 😉

[paulefoe]

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 ;)

[KostyaEsmukov]

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 🎉

[KostyaEsmukov]

Whoops, the base branch was wrong. It should have been master instead of distance-warnings. Cherry-picked to master as cddf65d