test(kernelcpd): exhaustive test for kernelcpd

[oboulant]

No description provided.

[codecov]

Codecov Report

Merging #108 (4890b03) into master (a9d1827) will increase coverage by 2.34%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
+ Coverage   92.77%   95.12%   +2.34%     
==========================================
  Files          41       41              
  Lines         955      964       +9     
==========================================
+ Hits          886      917      +31     
+ Misses         69       47      -22     

Impacted Files	Coverage Δ
src/ruptures/detection/kernelcpd.py	100.00% <ø> (+33.92%)	⬆️
src/ruptures/detection/binseg.py	100.00% <100.00%> (ø)
src/ruptures/detection/window.py	100.00% <100.00%> (ø)
src/ruptures/utils/bnode.py	96.15% <100.00%> (+0.50%)	⬆️
src/ruptures/__init__.py	100.00% <0.00%> (ø)
src/ruptures/detection/bottomup.py	100.00% <0.00%> (+2.77%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9d1827...4890b03. Read the comment docs.

[oboulant]

I modified the tests for the detection part in order to take into account the model argument in the pytest.mark.parametrize.
It creates some fails. I fixed some. Some remain.
2 major things to adress :

• For 1D constant signal test, the window detection algo returns a break points array of length 1 where it should be 2 (1 + the size of the signal). To be noted that when the signal is not constant, then the length of the returned array is fine
• There are some warnings at runtime with numpy : RuntimeWarning: invalid value encountered in double_scalars. It is always with the normal cost and constant signal (for Binseg, BottomUp-normal and Window-normal. Pelt and Dynp seem to be fine in this setup with normal cost). Possibility to modify numpy behavior on warnings with old_settings = np.seterr(all='raise'). See here

[oboulant]

As for the runtime warnings and the Binseg case, it comes from the fact that we take the log of the det (see here). Since the cost is 0, then log(0)=-inf, which is fine as long as we are not trying to add something to it (actually we do -inf - (-inf) - (-inf) here.
I will proceed with some modifications in Binseg._single_bkp to add an early stop in case segment_cost = self.cost.error(start, end) is -inf end return the appropriate values (as of now, the returned values are wrong)

[oboulant]

To be checked but all the problems raised above might be solved.

[deepcharles]

For 1D constant signal test, the window detection algo returns a break points array of length 1 where it should be 2 (1 + the size of the signal). To be noted that when the signal is not constant, then the length of the returned array is fine

To me, this is the correct behaviour: on constant signals, the returned list should be [n_samples]

[deepcharles]

There are some warnings at runtime with numpy : RuntimeWarning: invalid value encountered in double_scalars. It is always with the normal cost and constant signal (for Binseg, BottomUp-normal and Window-normal. Pelt and Dynp seem to be fine in this setup with normal cost). Possibility to modify numpy behavior on warnings with old_settings = np.seterr(all='raise')

This is because it computes the inverse of the variance (so 1/0 because the signal is constant). To me, this is a normal behaviour that we should let as is.

EDIT: oh ok, I should have read your last comment.

[deepcharles]

I do not think that modifying the search method is the correct answer to this issue, because this problems really is about CostNormal. This imposes some restrictions (quite unlikely to be triggered I admit) on all future cost functions.
I would rather add an if loop in the cost function method .error(start, end). Something like

_, val = slogdet(cov)
if np.isinf(val) and val<0:
    return 0

[deepcharles]

I will chek what is the behaviour of search methods (other than windows) on constant signals.

[oboulant]

I do not think that modifying the search method is the correct answer to this issue, because this problems really is about CostNormal. This imposes some restrictions (quite unlikely to be triggered I admit) on all future cost functions.
I would rather add an if loop in the cost function method .error(start, end). Something like

_, val = slogdet(cov)
if np.isinf(val) and val<0:
    return 0

I do not think it would work, methodologically speaking. Here is an example where the result of such a implementation would be false :

X = np.ones((100,1))
X[52] = 1.5

my_normal_cost = CostNormal().fit(X)

error_constant = my_normal_cost.error(0, 50)
error_non_constant = my_normal_cost.error(50, 100)

We would end up with error_constant > error_non_constant (0 > -5.318520073865557), which sounds contrary to what a Cost() should implement in terms of the underlying properties.

WDYT ?

[deepcharles]

you are right, good catch. Then your solution is clearly the best one.

[deepcharles]

LGTM