Add PBC capability to merge_split_MEST

[w-k-jones]

This turned into a general refactor of merge_split_MEST, with the following changes and additions:

1. Adds PBC capabilities, using the same BallTree distance search approach as used in feature detection.
2. Moves filtering by frame_len before calculating the minimum spanning tree, improving the number of merging/splitting cells linked particularly for long tracking time periods.
3. Changes the cell merging logic to use scipy.sparse.csgraph.connected_components, improving performance
4. Made sure all output arrays are int dtype, and started the track id from 1 rather than 0

I'm also going to look into adding a flag for whether a cell started with a split or ended with a merge, and which object it was merged into/split from

• Have you followed our guidelines in CONTRIBUTING.md?
• Have you self-reviewed your code and corrected any misspellings?
• Have you written documentation that is easy to understand?
• Have you written descriptive commit messages?
• Have you added NumPy docstrings for newly added functions?
• Have you formatted your code using black?
• If you have introduced a new functionality, have you added adequate unit tests?
• Have all tests passed in your local clone?
• If you have introduced a new functionality, have you added an example notebook?
• Have you kept your pull request small and limited so that it is easy to review?
• Have the newest changes from this branch been merged?

[w-k-jones]

Keeping as draft until #368 is merged as this contains the same commits

[kelcyno]

Oh this looks really interesting @w-k-jones I'll get started on it!

[w-k-jones]

Ok, I ended up going down a bit of a rabbit hole there. I found that because the filtering for frame_len was carried out after the minimum euclidean spanning tree calculation, valid links were being removed because a spatially closer cell at another point in time was being removed and therefore removing the edges between two neighbouring cells. This had more of an impact the longer the time period being tracked. When I swapped to filtering by frame_len before calculating the MEST it massively increased the number of linked cells, which then caused the rest of the merge/split routine to run slower. I've been experimenting with scipy.sparse.csgraph.connected_components recently, and this seemed like a really good application for linking the neighbouring cells into connected tracks

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (53df42d) 56.79% compared to head (e9ec119) 56.61%.
Report is 43 commits behind head on RC_v1.5.x.

Additional details and impacted files

@@              Coverage Diff              @@
##           RC_v1.5.x     #372      +/-   ##
=============================================
- Coverage      56.79%   56.61%   -0.18%     
=============================================
  Files             20       20              
  Lines           3435     3407      -28     
=============================================
- Hits            1951     1929      -22     
+ Misses          1484     1478       -6     

Flag	Coverage Δ
unittests	56.61% <100.00%> (-0.18%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[freemansw1]

I haven't started reviewing this yet, but I wonder if we shouldn't try to push this to v1.5.3? I'm starting to get antsy about v1.5.2 coming out.

[freemansw1]

Thanks for doing this, @w-k-jones; a critical improvement to an important piece of code.

I'm not that familiar with this code, but I've added in some thoughts here in a few places. Probably would be good to get a review in from @kelcyno.

[freemansw1]

I'm not sure what style is required here, but I think we should move from optional to say that the default is none.

[w-k-jones]

Looking through the main functions, I think we need to standardise a way of highlighting optional keywords and their default values. I'll update this for now by stating the default at the end of the description

[freemansw1]

In the rest of the 3D code, we support both constant vertical grid spacing and user-specified varying vertical grid spacing. We can either require constant vertical grid spacing for this code (less ideal) and check that the user didn't pass in an array-like, or update the code to support both (if it doesn't already). I'm happy enough with either outcome.

[w-k-jones]

I overlooked that functionality in the rest of the code before. I'll see if a similar system to that used in tracking can be used here. I think the same restriction as here may exist in the filtering of features by proximity in feature detection so I'll also look into resolving that

[freemansw1]

Same comment on optional as above.

[w-k-jones]

Reminder to myself: currently the PBC handling requires all of h1_min, h1_max, h2_min and h2_max to be provided even if PBCs are calculated over only one dimension. I should fix this to use default values for the dimension not being used before merge