Fixed test_centerline crashing due to bump in Python library

[jcohenadad]

Since recently, test_get_centerline is failing on Travis. This is probably related to an upgrade of a Python library and/or platform-dependent (as noted in #2686)

This PR:

• Replaced numpy.polyfit by numpy.polynomial.Polynomial as per numpy's doc, in centerline.curve_fitting.polyfit_1d()
• Removed: test_get_centerline_nurbs[img_ctl5-expected5-params5] due to a ReconstructionError caused by the change in the dummy centerline generation.

Fixes #2753

[jcohenadad]

Cannot reproduce failed test on a456fa5 on an Ubuntu 16.04 (bireli). Note: Python's libraries installed are the same (visual comparison from sct_check_dependencies)

Cannot reproduce on Docker 18.04 (all tests pass).
Cannot reproduce on proper 18.04 (rosenberg).

[jcohenadad]

This issue is likely related to #2686.

Instead of relaxing (again) the threshold, maybe the proper approach would be to remove the tests where fitting is badly conditioned (ie very few points). Currently the Travis tests fail for these cases:

test_get_centerline_polyfit[img_ctl3-expected3-params3] → size_arr=(9, 9, 9), subsampling=3
test_get_centerline_polyfit[img_ctl5-expected5-params5] → size_arr=(30, 20, 50), subsampling=1 (surprising! this is not so ill-defined...)
test_get_centerline_bspline[img_ctl2-expected2-params2] → size_arr=(41, 7, 9)
test_get_centerline_nurbs[img_ctl5-expected5-params5] → size_arr=(30, 20, 50), subsampling=1 (not ill-defined either...)

The following warning suggests this is indeed an issue:

unit_testing/test_centerline.py:61
  /home/travis/build/neuropoly/spinalcordtoolbox/unit_testing/test_centerline.py:61: RankWarning: Polyfit may be poorly conditioned
    (dummy_centerline(size_arr=(30, 20, 50), subsampling=5),

[jcohenadad]

Series of investigations on my OSX laptop:

Trying to replace numpy.polyfit with numpy.polynomial.Polynomial as per this suggestion, and numpy's doc:

The Polynomial.fit class method is recommended for new code as it is more stable numerically. See the documentation of the method for more information.

When replacing generation of dummy_centerline

=========================================================== 11 failed, 30 passed, 1257 warnings in 264.53s (0:04:24) ===========================================================

When using Polynomial also in centerline.core():

=========================================================== 11 failed, 30 passed, 1255 warnings in 250.04s (0:04:10) ===========================================================

[jcohenadad]

Looks like polyfit might actually provide more regularized outputs when extrapolating at edges. Example below:

polyfit:

Polynomial.fit:

[jcohenadad]

Given the extrapolation issue caused by polynomial fitting, it would probably be wise to replace a poly fit by a bspline fit for the ground-truth centerline.

However, we should keep the suggested Polynomial.fit()

[charleygros]

Looks great to me and investigations make sense.
Tests all green on my local machine.
Is there anything in particular you would like me to test / look at? Otherwise it looks ready to be merged!

[charleygros]

Would it be relevant to display the default value by default in order to reduce maintenance etc?
eg https://stackoverflow.com/a/12151325/13306686
If so, I will open a new issue

[jcohenadad]

Yes that would be awesome!

[charleygros]

There is actually specified in np.polyfit documentation:
The Polynomial.fit class method is recommended for new code as it is more stable numerically.

Makes sense :-)