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.