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.
How is PAA different from Downsampling? Based on the description, they sound the same
Reply via ReviewNB
| @@ -0,0 +1,839 @@ | |||
| { | |||
There was a problem hiding this comment.
The variables cc and rr are too generic and do not convey what they are keeping track of. Please consider changing. I can't really tell what this function is doing or why it is important.
Reply via ReviewNB
| @@ -0,0 +1,839 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #1. def computeKTIP(T, m, dsr0):
Maybe rename to def compute_ktip or compute_KTIP
Reply via ReviewNB
| @@ -0,0 +1,839 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #1. # Testing the KTIP algorithm
How do we know what the output should be and why?
Reply via ReviewNB
| @@ -0,0 +1,839 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #1. def PAA(T, dsr):
I don't like dsr. Can we use rate or downsample instead? Even sampling_rate is okay
Reply via ReviewNB
| @@ -0,0 +1,839 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #15. prnT = [] # Initialize an empty list to store pruned subsequences
Please use more expressive naming like pruned_T or something
Reply via ReviewNB
| @@ -0,0 +1,839 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,839 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -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! |
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!