Fix #844 Fix bug in module motifs

[NimaSarajpoor]

@seanlaw
Sorry for the noise! I am not getting revenge btw 😆 I am just newbie. So, the nature of our noises are different :)

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (82ce308) 99.28% compared to head (7e549bd) 99.28%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #856   +/-   ##
=======================================
  Coverage   99.28%   99.28%           
=======================================
  Files          83       83           
  Lines       13699    13753   +54     
=======================================
+ Hits        13601    13655   +54     
  Misses         98       98           

Impacted Files	Coverage Δ
stumpy/motifs.py	100.00% <ø> (ø)
tests/test_motifs.py	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[NimaSarajpoor]

For future reference:
Fixing the bug resolves the exposed error and exposes an issue in one of the existing test functions. We are going to fix it in the next commit.

[NimaSarajpoor]

@seanlaw
I just left two comments. Please add your comments as well if you have any.

[seanlaw]

I never noticed the decimal=4 in some of the npt.assert_almost_equal and I wonder if we even need to specify it? There are multiple instances throughout this file.

Also, maybe we should move the naive portion out into its own def naive_motifs function above def naive_match function

[NimaSarajpoor]

I never noticed the decimal=4 in some of the npt.assert_almost_equal and I wonder if we even need to specify it? There are multiple instances throughout this file.

I will remove it. If I notice an error raised by a specific test(s) as a result of such change, I will create an issue so that we can handle that specifc test(s) in another PR.

Also, maybe we should move the naive portion out into its own def naive_motifs function above def naive_match function

Correct. Later, we can re-use it for testing T_subseq_isconstant.

Note that the max_distance and cutoff parameters are hard-coded and they are both set to np.inf. We can add them to naive function as arguments (so that they can match the signature of performant func more closely) at the cost of making the naive func more complicated.

[seanlaw]

@NimaSarajpoor Is this ready to be merged?

[NimaSarajpoor]

@seanlaw
I think it is ready to be merged.

---

FYI: In the function naive_motifs, we can add the following if-check to make sure that T is long enough for our test.

k = len(T) - m + 1
if k < m * max_matches * max_motifs:
      raise ValueError(...)

I think m * max_matches * max_motifs is a good, weak lower bound for k. However, since it is a naive function, I decided to avoid raising error, and instead, I just added a comment regarding this matter.

[seanlaw]

Thanks @NimaSarajpoor