Fixed #1014 Fix max_matches=None bug

[ejorgensen-wl]

This fix addresses issue #1014 by not slicing the query_matches if max_matches is set to None for motifs (which in turn calls _motifs with max_matches=np.inf.

I made an initial effort to add testing, but I'm sure there are better ways to do so.

Pull Request Checklist

• Fork, clone, and checkout the newest version of the code
• Create a new branch
• Make necessary code changes
• Install black (i.e., python -m pip install black or conda install -c conda-forge black)
• Install flake8 (i.e., python -m pip install flake8 or conda install -c conda-forge flake8)
• Install pytest-cov (i.e., python -m pip install pytest-cov or conda install -c conda-forge pytest-cov)
• Run black . in the root stumpy directory
• Run flake8 . in the root stumpy directory
• Run ./setup.sh && ./test.sh in the root stumpy directory
• Reference a Github issue (and create one if one doesn't already exist)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.42%. Comparing base (3077d0d) to head (9e0b2ce).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1015   +/-   ##
=======================================
  Coverage   97.41%   97.42%           
=======================================
  Files          89       89           
  Lines       14922    14934   +12     
=======================================
+ Hits        14537    14549   +12     
  Misses        385      385           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[seanlaw]

@ejorgensen-wl Thanks again for identifying this bug. The decision to do:

https://github.com/TDAmeritrade/stumpy/blob/3077d0ddfb315464321dc86f8ec3bf2cab9ce3b1/stumpy/motifs.py#L336-L337

was decided a long time ago and, in hindsight, that was a poor choice. Instead, I think we could've/should've been more specific and done:

if max_matches is None:  # pragma: no cover
    max_matches = P.shape[-1]

P.shape[-1] is an integer that would be the same as the maximum number of subsequences in T (i.e., the standard T - m + 1 number of subsequences) that are possible and is a more explicit number than np.inf, which isn't even an integer (it's a floating point value!).

What do you think about this remedy instead? Would this work?

[ejorgensen-wl]

@ejorgensen-wl Thanks again for identifying this bug. The decision to do:

https://github.com/TDAmeritrade/stumpy/blob/3077d0ddfb315464321dc86f8ec3bf2cab9ce3b1/stumpy/motifs.py#L336-L337

was decided a long time ago and, in hindsight, that was a poor choice. Instead, I think we could've/should've been more specific and done:

if max_matches is None:  # pragma: no cover
    max_matches = P.shape[-1]

P.shape[-1] is an integer that would be the same as the maximum number of subsequences in T (i.e., the standard T - m + 1 number of subsequences) that are possible and is a more explicit number than np.inf, which isn't even an integer (it's a floating point value!).

What do you think about this remedy instead? Would this work?

That's an excellent and much cleaner solution than what I was thinking, thanks! I implemented your suggestions in my latest commit.

[seanlaw]

That's an excellent and much cleaner solution than what I was thinking, thanks! I implemented your suggestions in my latest commit.

@ejorgensen-wl Thank you for your patience and for working through this with us! We really appreciate the PR. Once the tests all pass, I'll merge the PR (unless you had anything further to add?).

[ejorgensen-wl]

I like it as is - thanks!

[seanlaw]

@ejorgensen-wl Thank you again for the excellent PR! I hope you will consider contributing more!