implemented majority of MOMP [#1031] #1046
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1046 +/- ##
=======================================
Coverage 97.33% 97.33%
=======================================
Files 89 89
Lines 15027 15027
=======================================
Hits 14626 14626
Misses 401 401 ☔ View full report in Codecov by Sentry. |
| @@ -0,0 +1,839 @@ | |||
| { | |||
There was a problem hiding this comment.
Is this example from the paper? If not, then instead of using a random input, please try to reproduce something (i.e., a figure) from the paper where possible. This is the key deliverable in a "notebook reproducer" as it allows us to visually see if we have successfully reproduced the work without needing to dive into the code (yet).
Reply via ReviewNB
| @@ -0,0 +1,839 @@ | |||
| { | |||
There was a problem hiding this comment.
What happens when we make the time series of length 1 million and a motif length of 50? How long does momp take?
Reply via ReviewNB
|
@joehiggi1758 I've left some comments and suggestions for you to consider. Thank you for all of your hard work! |
| @@ -0,0 +1,839 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #24. dist = np.sqrt(np.sum((T[rr:rr + m] - T[cc:cc + m]) ** 2))
The authors used the name "ED" for the distance function in the table 3. Did they mean Euclidean Distance or z-normalized Euclidean Distance? If it is the latter, then I think this line of code needs to be revised.
Reply via ReviewNB
| @@ -0,0 +1,839 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #27. dist = amp[i] - ip[i] - ip[j]
Since amp contains z-normalized distances here, IMO it makes sense to say that ip[i] and ip[j] should be z-normalized distances. Therefore, I think the distance computed in computeKTIP should be the z-normalized distance. Better to check paper to make sure of it.
Reply via ReviewNB
| @@ -0,0 +1,839 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #29. mp, _ = stumpy.mstump(stacked_segments, m) # Compute the matrix profile for the stacked segments
mstump is multi-dimensional stump. Does the paper try to analyze the two segments in multi-dimensional way? (I do not know what that means here since mstump is complicated)
Or, does it try to compute the mp for the subsequences in those two segments, something like stumpy.stump(np.r_[segA, np.nan, segB], m) ?
Reply via ReviewNB
There was a problem hiding this comment.
Thanks @NimaSarajpoor. I totally missed this. I'm guessing that msump is NOT what we want here!
|
@joehiggi1758 |
Pull Request Checklist
Below is a simple checklist but please do not hesitate to ask for assistance!
black(i.e.,python -m pip install blackorconda install -c conda-forge black)flake8(i.e.,python -m pip install flake8orconda install -c conda-forge flake8)pytest-cov(i.e.,python -m pip install pytest-covorconda install -c conda-forge pytest-cov)black --exclude=".*\.ipynb" --extend-exclude=".venv" --diff ./in the root stumpy directoryflake8 --extend-exclude=.venv ./in the root stumpy directory./setup.sh dev && ./test.shin the root stumpy directoryHey @seanlaw - hope you had a great Saturday!