Adds normalization of lat/lon to their corresponding ranges

[merschformann]

Description

Adds normalization of lat/lon values outside the allowed ranges. I.e., latitude values outside of [-90,90] and longitude values outside of [-180,180] will be moved into the corresponding range.

This behavior is debated in #49. Another alternative would be to simply throw an exception on lat outside of [-90,90] or lon outside of [-180,180]. Hence, feel free to reject and use a different option.

Notes

• The test expectations had to be slightly adjusted due to numerical issues

[merschformann]

There seems to be a dependency installation error unrelated to the changes here. I also get it locally on the main branch and first commits still succeeded.

I can reproduce on main branch when running pipenv install --dev.

🤔

[jdeniau]

The dependency issue is probably due to untitaker/python-atomicwrites#61

I don't even know why is this package used ¯_(ツ)_/¯

[jdeniau]

@uri-rodberg What do you think of this ?

Personally I'm not really fan of the normalize parameter, but as you two probably use more haversine than I do, I'll let you choose.

[jdeniau]

It seems that atomicwrites has been used by pytest, and removed in pytest-dev/pytest#10115. I will try to fix the build

[jdeniau]

@merschformann The dependency issue is fixed. I updated your branch, but it seems that you now have an assertion error in your test.

[merschformann]

@jdeniau Thanks for fixing! ❤️

Personally I'm not really fan of the normalize parameter, but as you two probably use more haversine than I do, I'll let you choose.

I also don't have a strong opinion at all. I think I lean towards only raising the exception on improper lat/lon inputs a little. It would keep the code base simpler. But I'd like to defer to others on making the final call. (ping @uri-rodberg and who else is interested)

However, I am happy to quickly adapt the PR after we come to a decision.

[uri-rodberg]

@uri-rodberg What do you think of this ?

Personally I think it's a good idea to let the user decide if to raise an exception or normalize lat/lon. So I prefer to keep it this way.

I would also add the following tests:

    with pytest.raises(ValueError):
        haversine((-90.0001, 0), (0, 0), Unit.DEGREES)
    with pytest.raises(ValueError):
        haversine((0, 0), (90.0001, 0), Unit.DEGREES)
    with pytest.raises(ValueError):
        haversine((0, -180.0001), (0, 0), Unit.DEGREES)
    with pytest.raises(ValueError):
        haversine((0, 0), (0, 180.0001), Unit.DEGREES)

(use default value for normalize)

And:

# assert that haversine((-90.0001, 0), (0, 0), Unit.DEGREES, normalize=True) == haversine((-89.9999, 180), (0, 0), Unit.DEGREES, normalize=True) # or that the difference between them is very close to 0
# assert that haversine((-90.0001, 30), (0, 0), Unit.DEGREES, normalize=True) == haversine((-89.9999, -150), (0, 0), Unit.DEGREES, normalize=True) # or that the difference between them is very close to 0
# Also check haversine((0, 0), (90.0001, 0), Unit.DEGREES, normalize=True)
# Also check haversine((0, 0), (90.0001, 30), Unit.DEGREES, normalize=True)
# Also check haversine((0, -180.0001), (0, 0), Unit.DEGREES, normalize=True)
# Also check haversine((30, -180.0001), (0, 0), Unit.DEGREES, normalize=True)
# Also check haversine((0, 0), (0, 180.0001), Unit.DEGREES, normalize=True)
# Also check haversine((0, 0), (30, 180.0001), Unit.DEGREES, normalize=True)

[merschformann]

Added the test cases. Please check whether I understood them correctly and let me know, if something is missing. ☺️

[uri-rodberg]

Added the test cases. Please check whether I understood them correctly and let me know, if something is missing. ☺️

Thanks, it looks fine to me.

[uri-rodberg]

Just a question - why do you use 179.99990000000003 and not 179.9999?

[merschformann]

I was expecting numerical issues, since we're checking equality. Apparently, it's not necessary.
Also, I agree that using pytest.approx would be cleaner in that case.

[merschformann]

Let me cleanup the test cases a bit in general...

[jdeniau]

Thanks to both of you, this has been released as 2.6.0