-
Couldn't load subscription status.
- Fork 337
implemented majority of MOMP [#1031] #1046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 97.33%. Comparing base (
9616686) to head (da0739a).
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.
📢 Have feedback on the report? Share it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @NimaSarajpoor!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @NimaSarajpoor. I totally missed this. I'm guessing that mstump is NOT what we want here!
@joehiggi1758
Great work on implementing many algorithms including the ones referenced and used in the paper!! A few things got my attention as I was going through your notebook. I provided some comments to bring them to your attention.
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!