Improve linear splining

[SylviaWhittle]

Closes #607

This PR aims to improve the splining of linear molecules. Previously we just skipped splining for linear molecules, however @Jean-Du got it working through use of the smoothing and periodicity parameters to scipy's splprep (splining) function.

I wanted to make get_splined_traces() into a function rather than a method, since it doesn't really need the self keyword in it and could just as easily take all its arguments as parameters (better for documentation too!) so I've left it in a functional style, but still being a method in case we want to switch it later on.

I've also added unit tests for get_splined_traces() 😄

I've had to update a lot of tests for this change. I have NOT sanity checked all of them, but I have plotted the effects on the splined traces of minicircle.spm:

Previous splines:

Updated splines:

Thanks to @Jean-Du for the code here, I just copied it across, applied pylint adherence and updated / created tests.

[Jean-Du]

Closes #607

[ns-rse]

Some probably not very well formed thoughts.

I've made a note to ensure I rebase for the branch I'm working on the pruning on.

[MaxGamill-Sheffield]

Apologies I've not gotten around to looking at this but hope too soon.

I just want to check if you've managed to solve the oscillating issue causing whacky traces? I'll try to find a test file to see if this still occurs.

[codecov-commenter]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (cd0cf4d) 83.53% compared to head (daebcdc) 83.82%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #653      +/-   ##
==========================================
+ Coverage   83.53%   83.82%   +0.28%     
==========================================
  Files          21       21              
  Lines        3061     3072      +11     
==========================================
+ Hits         2557     2575      +18     
+ Misses        504      497       -7     

Files	Coverage Δ
topostats/processing.py	93.42% <100.00%> (ø)
topostats/validation.py	100.00% <ø> (ø)
topostats/tracing/dnatracing.py	72.87% <95.00%> (+2.90%)	⬆️

... and 1 file with indirect coverage changes

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

[MaxGamill-Sheffield]

Sorry if this has been waiting on me, I've been testing to see if we see unwanted oscillations in the linear splines like we do in the nodeStats branch, i.e. the following which give incorrect contour lengths:

In the nodestats branch it may be due to the height-biased skeletonisation differences. I'll ignore this issue for now and I've not seen it occur for this branch but just a note for the future the solution may be to just use the ordered traces as these will already be height-aligned.

[MaxGamill-Sheffield]

Code looks good and clean. Thanks for making this change so quick and sorry for the lack of review on this until now.

One thing I want to ask is why some of the circular molecule lengths have changed? My thought is that it shouldn't be touched as only the params (and therefore splining) of the linear mols have changed? The amounts are small enough that it shouldn't be too much of an issue - is it the step_size?.

I also am curious about the shepherd hooks you have for the minicircles image and why despite they don't terminate on or nearer the trace - is it because of the spline averaging where the last coordinate is only obtained for 1 of 4 splines and as such has less weighting? Or am I overcomplicating this hahaha

[SylviaWhittle]

Just a note, mainly for future me: The linear molecules look different because previously we just returned the non-splined, fitted traces since we didn't spline the linear traces. We now spline them so they are different.

[SylviaWhittle]

I discovered and fixed a bug that caused splining to fail in some cases where there are duplicate, consecutive points in the sampled points arrays. Added a function to remove these duplicates and added tests for it. This results in grains succeeding and hence the change in output columns.

[SylviaWhittle]

Oh and @ns-rse I added a contingency (plus test) to your trace_mask function, where it will skip grains that have pixels outside the bounds of the image. This happens to some splined traces and so to utilise your function I needed to add a contingency to it. (Tested of course).

[SylviaWhittle]

Sorry if this has been waiting on me, I've been testing to see if we see unwanted oscillations in the linear splines like we do in the nodeStats branch, i.e. the following which give incorrect contour lengths:

In the nodestats branch it may be due to the height-biased skeletonisation differences. I'll ignore this issue for now and I've not seen it occur for this branch but just a note for the future the solution may be to just use the ordered traces as these will already be height-aligned.

I don't think this has been fixed, as the actual splining functionality has not changed. This could be a new issue if it needs addressing.

[ns-rse]

@MaxGamill-Sheffield the difference in circular molecule stats is due to a suggested change by @Jean-Du to set the smoothing of circular molecules to 2.0 (previously 0.0). I have reverted it back to 0.0 now just for simplicity. We can always change it later.

I've not digged in to check but if this is a parameter it would make sense for it to be configurable, with a sensible default.

[SylviaWhittle]

@MaxGamill-Sheffield the difference in circular molecule stats is due to a suggested change by @Jean-Du to set the smoothing of circular molecules to 2.0 (previously 0.0). I have reverted it back to 0.0 now just for simplicity. We can always change it later.

I've not digged in to check but if this is a parameter it would make sense for it to be configurable, with a sensible default.

It's in the config now 👍

[ns-rse]

Sorry I've taken so long to get round to reviewing this, its a big PR.

Some minor comments but nothing blocking this from being merged.

[ns-rse]

Good to go ✅ 👍

No substantive code changes requested, @SylviaWhittle has addressed aspects of changes in length in other comments.