param `s` in prescrump and scrump raises error when set to default and T_B is not None

[NimaSarajpoor]

So, I have been working on scrump and prescrump, and I think we can get error if s=None (default) and T_B is not None.

Because we do:

# in prescrump

# lines 330-334
if T_B is None:
 T_B = T_A
 excl_zone = int(np.ceil(m / config.STUMPY_EXCL_ZONE_DENOM))
 else:
 excl_zone = None

# lines 342-343
if s is None: # pragma: no cover
 s = excl_zone

So, when T_B is not None, excl_zone is set to None, which makes s=None if s is not provided by the user.

---

I also tried an example to make sure of it:

import numpy as np
from stumpy.scrump import prescrump

T_A = np.random.rand(10)
T_B = np.random.rand(10)
m = 3
prescrump(T_A, m, T_B)

And the error:

83 if s is None: # pragma: no cover
 84 s = excl_zone
---> 86 indices = np.random.permutation(range(0, l, s)).astype(np.int64)
 88 return (T_A, T_B, μ_Q, σ_Q, M_T, Σ_T, indices, s, excl_zone)

TypeError: 'NoneType' object cannot be interpreted as an integer

---

Solution:
Maybe this suffices:

if s is None: # pragma: no cover:
 s = int(np.ceil(m / config.STUMPY_EXCL_ZONE_DENOM))

[seanlaw]

Hmm, it's a bit weird. An exclusion zone is only defined when we are performing a self-join. In the case of an AB-join, an exclusion zone makes no sense. At the end of the day, s is simply some spacing in which to traverse along the time series. I think it should be:

if s is None: # pragma: no cover:
 if excl_zone is not None:
 s = excl_zone
 else:
 s = int(np.ceil(m / config.STUMPY_EXCL_ZONE_DENOM))

What do you think?

[NimaSarajpoor]

An exclusion zone is only defined when we are performing a self-join. In the case of an AB-join, an exclusion zone makes no sense. At the end of the day, s is simply some spacing in which to traverse along the time series

Exactly! You are right! That is why I simply did int(np.ceil(m / config.STUMPY_EXCL_ZONE_DENOM)). The one you wrote seems clearer in terms of readability. Still, the line s = int(np.ceil(m / config.STUMPY_EXCL_ZONE_DENOM)) might sound a little bit strange since one can notice config.STUMPY_EXCL_ZONE_DENOM and think "what is excl_zone doing here?".

---

Maybe we add STUMPY_SAMPLING_DENOM in config:

# in config.py

STUMPY_SAMPLING_DENOM = STUMPY_EXCL_ZONE_DENOM

And, then we just need to explain it in the docstring. And, we use config.STUMPY_SAMPLING_DENOM as default of denom
when s is None.

Update:
This is just to avoid confusion. But, I now THINK it adds a complexity to the code as reader now should think about the value of config.STUMPY_SAMPLING_DENOM! so, feel free to ignore this suggestion :)

---

FYI: Currently, the docstrings in scrump.py say:

# in prescrump
s : int, default None
 The sampling interval that defaults to
 `int(np.ceil(m / config.STUMPY_EXCL_ZONE_DENOM))`

# in class scrump
s : int
 The size of the PreSCRIMP fixed interval. If `pre_scrump=True` and `s=None`,
 then `s` will automatically be set to
 `s=int(np.ceil(m / config.STUMPY_EXCL_ZONE_DENOM))`, the size of the exclusion
 zone.

I think the former is more precise. The latter description mentions "exclusion zone", which, as you said, is not meaningful for AB-join.

[seanlaw]

I think the former is more precise. The latter description mentions "exclusion zone", which, as you said, is not meaningful for AB-join.

I think we're overthinking it. I think it's probably fine to do what I suggested even if it is an AB-join.

[seanlaw]

@NimaSarajpoor I think I closed the PR too soon. It looks like we need to handle:

Name Stmts Miss Cover Missing
-------------------------------------------------
stumpy/scraamp.py 142 1 99% 519
stumpy/scrump.py 145 1 99% 588
-------------------------------------------------
TOTAL 11443 2 99%

[NimaSarajpoor]

@seanlaw
I will fix it soon.