Add enhanced support for geographic coordinates

[santisoler]

Add latlon_continuity() function that moves a set of longitudinal coordinates either
to the [0, 360) or [-180, 180) degrees intervals, depending which one is more suited for
that particular set of coordinates.
It also allow to modify an extra set of coordinates based on the decision made on the
first set.
Modify the inside() function to enhance how the inside points are selected, even if
their longitudinal coordinates are not defined on the same degree interval.

Fixes #171

Reminders

• Run make format and make check to make sure the code follows the style guide.
• Add tests for new features or tests that would have caught the bug that you're fixing.
• Add new public functions/methods/classes to doc/api/index.rst and verde/__init__.py.
• Write detailed docstrings for all functions/classes/methods.
• If adding new functionality, add an example to the docstring, gallery, and/or tutorials.

[welcome]

💖 Thanks for opening this pull request! 💖

Please make sure you read our Contributing Guide and abide by our Code of Conduct.

A few things to keep in mind:

• Remember to run make format to make sure your code follows our style guide.
• If you need help writing tests, take a look at the existing ones for inspiration. If you don't know where to start, let us know and we'll walk you through it.
• All new features should be documented. It helps to write the docstrings for your functions/classes before writing the code. This will help you think about your code design and results in better code.
• No matter what, we are really grateful that you put in the effort to do this! ⭐

[leouieda]

@santisoler thanks for taking a shot at this! A few things to think about:

1. Does latlon_continuity need to be public? Doesn't seem like it will be very useful outside of inside or similar functions. It might be better to keep it private until we have a clear use case for it.
2. This sort of thing is tricky and has loads of corner cases. I'd suggest making some more extensive tests (apart from doctests), particularly for corner cases (like regions around the poles or possibly overlapping on the other side of the globe like -200/200)

As a general rule, I find it useful to write the example for the feature first. That way I quickly see the usability problems and if I can't think of an example, then it probably isn't needed. It's always easier to make a private function public than the other way around.

[santisoler]

Thanks @leouieda for the review.

1. Does `latlon_continuity` need to be public? Doesn't seem like it will be very useful outside of `inside` or similar functions. It might be better to keep it private until we have a clear use case for it.

I think you're right, it should be private, at least for now.

2. This sort of thing is tricky and has loads of corner cases. I'd suggest making some more extensive tests (apart from doctests), particularly for corner cases (like regions around the poles or possibly overlapping on the other side of the globe like `-200/200`)

Thanks for pointing this out. Writing test functions is tricky. First you think how the function should work, then spend time writing it and optimizing it. And then you have to switch your brain to what could go wrong with it. It's not easy work and we often need another pair of eyes.
I'll add more tests functions for such cases.

As a general rule, I find it useful to write the example for the feature first. That way I quickly see the usability problems and if I can't think of an example, then it probably isn't needed. It's always easier to make a private function public than the other way around.

That's a nice way to start to write a new function. I'll try to implement it in the future.

[santisoler]

@leouieda I've rewritten the _lonlat_continuity function. Initially I was thinking to be a general function that could be used by other ones besides inside, but so far it is the only function which makes use of it.
So I decided to simplify it. If other functions would need similar capabilities, we should leave that for the future.
Moreover, I rewrote some tests and add new ones, for example with overlapping coordinates and the cases on the poles.

I thought this enhancement would be straight forward, but each time I dive into it I find rough points.
Can you review it now?

[santisoler]

@leouieda I've made some of the proposed changes.
I like how the code is looking now, although I should add a few more tests.
But I think the code is now cleaner and the usage is simpler.
The longitude_continuity accepts coordinates=None as argument, although I haven't added it to the docs.
Just wanted to know what do you think about letting this option, i.e.

longitude_continuity(coordinates=None, region=region)

Please, take a look at it and let me know if I should change anything else or if I'm missing something.

[leouieda]

Just wanted to know what do you think about letting this option, i.e.

Yep, the functionality looks good and the function does what we want. The only thing I'm entirely sure about is the function returning 1 or 2 things depending on input. I think this might come back to haunt us later. I'm leaning towards always returning coordinates, region even if coordinates is None. What do you think?

I think we talked about this but I can't remember: do we need coordinates=None right now or is this something that can be added later? If not, then I suggest we require coordinates and add this later if the need arises.

Do you have a sample dataset in mind that we could use for this? I can look for data around longitude 0 somewhere if not.

[santisoler]

Yep, the functionality looks good and the function does what we want. The only thing I'm entirely sure about is the function returning 1 or 2 things depending on input. I think this might come back to haunt us later. I'm leaning towards always returning coordinates, region even if coordinates is None. What do you think?

I have the same doubt regarding the 1 or 2 outputs. I think we should return coordinates, region on every case to avoid problems in the future.
We could add an example showing how to handle the coordinate=None case, something like:

_, new_region = vd.longitude_continuity(None, region)

I think we talked about this but I can't remember: do we need coordinates=None right now or is this something that can be added later? If not, then I suggest we require coordinates and add this later if the need arises.

Yeah! The tesseroid implementation on Harmonica is a perfect example where you don't mind about the coordinates, you just want to adjust the boundaries of the tesseroid (remember that the computation point coordinates and tesseroid boundaries are merged inside the kernels, where the angles are always arguments of trigonometric functions, so we don't mind about phases).

Do you have a sample dataset in mind that we could use for this? I can look for data around longitude 0 somewhere if not.

Any dataset could be useful. For example, ETOPO1 is on the [-180, 180) interval. We could make two examples from them:

1. Use a region on the [0, 360) interval, e.g. [200, 240].
2. Then use a region around the 0 with the modified coordinates on the previous point.

Although I think finding a dataset defined on [0, 360) and try to use a region on [-180, 180) would be better.

[leouieda]

Hi @santisoler, left a few comments here with minor things. What do we need to get this merged? The API looks good to me and the functionally seems to all be there. The test coverage is missing a fit and there are some CI errors. We could try to move this forward soon to make a new Verde release.

[santisoler]

@leouieda Now coordinates.py meet the 100% coverage and pylint doesn't not raise any errors.
The Azure Windows build is complaining because test_spline_warns_underdetermined fails. I'm almost sure this PR doesn't break anything related with spline, but I'm might be wrong.

Let me know if something else should be done here, otherwise I think this PR it's ready to be merged!

[leouieda]

I'm almost sure this PR doesn't break anything related with spline, but I'm might be wrong.

That is unrelated and a bit of a bad test now that I think about it. I don't know why it's complaining so much actually. Probably an update of numba that issues a new warning. I'll see if I can fix this in another PR before merging this. If not, then I'll merge as is.

Thanks for implementing this!