Convert trace.cpp to Cython

[QuLogic]

Rationale

This converts trace.cpp into Cython. The main reason for this is that it makes it easier to tweak things around a bit. For example, we could try out Cython's parallel loops, or change explicit GEOS calls to Shapely (which I am currently experimenting with to fix #805.)

Implications

Running against #1250 (to reduce network effects), I don't see any significant change in testing time, so I don't think there are any performance impacts from this change.

[pelson]

Interesting timing - I've been hacking around with _trace.cpp for the first time in a while over the weekend...

One of the implications of this change is that it becomes harder to imagine producing a libcartopy.so which does geometry projection without the need of a Python interpreter. Truthfully though a lot of the smarts exist in Python and _trace.cpp is of pretty limited value without it. Summary: Not a blocker.

Performance is naturally a concern which will be hard to measure comprehensively. Poor-man's performance testing using our existing test suite is a reasonable first stab - it is important to remember that we (should) have optimised tests to cover the necessary components in the shortest time possible - we have not optimised them to highlight performance regressions. Perhaps you could devise a few worst-case test-cases and measure the impact? Having said all of this, we have done little to improve the performance of this part of the code in quite some time - if moving to cython gives us the opportunity to experiment with optimisations more quickly, then it is reasonable to assume that this change will lead to faster code in the medium-to-long term. Summary: Not a blocker.

The reviewability of this change is pretty low - the wholesale change of language for this piece of code pretty much rules out line-by-line iterative review. That said, this code is being run throughout the test suite - if there were significant functional changes, we would see them in the tests failing. Summary: Not a blocker, though likely to lead to some churn of issues for users.

In summary

This change makes a lot of sense. There are a few downsides, but they are minor, especially compared to the benefits that the change unlocks. Thanks for this awesome PR @QuLogic - this is a really nice tidy of the existing functionality! 👍 🎆

P.S. FWIW I don't believe the fix to 805 is one that has to happen in the cartopy codebase. Of course, I want to minimise incompatibility (hopefully that goes without saying!) but it is entirely unreasonable to assume a global monopoly on a shared object - if a business were to do such a thing it would be called anti-competitive...

[pelson]

A couple of follow-ups:

• Could you take a look at pulling together a few worst-case performance tests and share the result here? Doesn't have to be fancy, but if you are keen we could start using ASV.
• The cdef functions migrated here are all nogil-able. Let's do that (I'm sure that was next on the list if you are investigating prange).

[QuLogic]

Re: reviewability: Obviously, it's already been merged, but I did convert each function in a separate commit if you feel like perusing that at your leisure. As to performance, I did try to evaluate it after each function was converted, so it shouldn't be too bad. I added as many type annotations as necessary to ensure things remained as simple as possible. The generated code is not the easiest to read, but I think the end result is fairly close to the original code (with a slight overhead).

I did forget to mention one other refactor: I moved the geodesic and proj function definitions to separate (definition-only) files; this was because they were re-used in a couple Cython llibraries and I wanted to consolidate the defintiion. Hopefully, this will also be helpful when we work on #1140.

[QuLogic]

Re: performance: I actually realized I was doing an improper test, running with -n4 (as slower tests might just be hidden on a different thread), so I redid it without xdist. Since vcr is not quite ready, I ran with -m 'not network' over ten runs.

The results are that 17cadcd (pre-merge) takes 58.6 ± 1.6 and master (427de4f) takes 57.4 ± 2.0. That's surprisingly a tiny bit faster, but also within the standard deviation of both, so not likely significant.

I also pulled the example from #325 (it's not an infinite loop anymore, but it's probably one of the slower transforms that could be done,) and ran it through bench:

# pre-merge
benchmarking python3 ../cp-issues/issue325.py
time                 919.7 ms   (914.3 ms .. 925.2 ms)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 928.6 ms   (926.3 ms .. 932.9 ms)
std dev              3.689 ms   (0.0 s .. 3.793 ms)
variance introduced by outliers: 19% (moderately inflated)

# post-merge
benchmarking python3 ../cp-issues/issue325.py
time                 924.2 ms   (918.8 ms .. 935.0 ms)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 918.6 ms   (915.7 ms .. 920.8 ms)
std dev              3.312 ms   (0.0 s .. 3.766 ms)
variance introduced by outliers: 19% (moderately inflated)

which seems pretty close.

[pelson]

Thanks for following up @QuLogic. I think we can be quite confident in this change - it is really quite impressive that you managed to migrate to cython without adding any noticeable overhead. As you can see from some of my follow-up, it has made a bunch of things significantly easier - including improving the test coverage of the bowels of the algorithm.

Once we have merged #1256, it would make sense to try to pull the cases you have evaluated here into the benchmark suite. Are you happy to do that, or would you like me to do it as part of #1256?