-
Notifications
You must be signed in to change notification settings - Fork 337
Fix issue 358 #407
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
Fix issue 358 #407
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Also, please ignore the ton of commits. They are just some residue. I guess you will squash the history in the end. |
I'm not entirely sure but let me take a look and see if I can understand what you mean. |
@mexxexx I went through the tutorial really, really quickly and I love what you've done and how you've laid everything out! Great work! I will need to find some time to go over it in more detail. Regarding the
Then, internally inside of |
Sure, take your time and then we can see if anything needs adjustment.
That is what I needed :) I did not think about setting the denominator to What about adding a small context manager to
Then one could to the following:
And then it would automatically reset to the original value:
However, I realize that this might be a bit over the top and only for a very specific use case. |
Initially, I thought that using a context manager was an interesting idea. However, as I thought through it:
It feels a lot more compact and explicit to do:
My vote is to not use a context manager. |
Also, if you wouldn't mind handling this, I've added a Matplotlib style file so the lines:
can now be replace with:
|
View / edit / reply to this conversation on ReviewNB seanlaw commented on 2021-06-22T01:52:15Z Please replace |
View / edit / reply to this conversation on ReviewNB seanlaw commented on 2021-06-22T01:52:15Z Please replace |
View / edit / reply to this conversation on ReviewNB seanlaw commented on 2021-06-22T01:52:16Z maybe "STUMPY provides you with a super powerful function called |
View / edit / reply to this conversation on ReviewNB seanlaw commented on 2021-06-22T01:52:16Z When you said "Above" I thought you meant in the last paragraph so maybe let's be specific and say, "Earlier, manually sorted the distance profile..."
"patients" should be "patient's"
"For example, if you have EEG data of a patients heartbeat and want to match one specific beat, then you may consider using a smaller threshold since your time series may be highly regular."
"Let's plot all of the discovered matches to see if we need to adjust our threshold" |
View / edit / reply to this conversation on ReviewNB seanlaw commented on 2021-06-22T01:52:17Z 'While some of the main feature are somewhat conserved across all subsequences, there seems to be a lot of artifacts as well.
With
Typically, one has to experiment a bit with what an acceptable maximum distance so let's try to change it to "four standard deviations below the mean" (i.e., a smaller maximum distance). |
View / edit / reply to this conversation on ReviewNB seanlaw commented on 2021-06-22T01:52:17Z Please change |
View / edit / reply to this conversation on ReviewNB seanlaw commented on 2021-06-22T01:52:18Z Looks like there is an error :( |
View / edit / reply to this conversation on ReviewNB seanlaw commented on 2021-06-22T01:52:19Z Please use |
@mexxexx I really enjoyed your addition! Great work! I provided some minor edits for you to consider |
We are adding a Binder link to the top of each tutorial. Would you mind adding the following below the main
Right now it links to |
Codecov Report
@@ Coverage Diff @@
## main #407 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 35 35
Lines 2764 2765 +1
=========================================
+ Hits 2764 2765 +1
Continue to review full report at Codecov.
|
@seanlaw I finally got around integrating your feedback into the tutorial. What do you think? |
@mexxexx I think everything looks great! I find some minor things but I can go over it after this PR gets merged. Thank you for this contribution! |
Hi @seanlaw , I installed the latest version of
Here is the error message:
I think that's because the stumpy's matplotlib file cannot be found. Do you know how to solve this problem? Thanks so much for your help! |
@Meng6 I think what you are experiencing is a separate issue. Would you mind posting your question in our Github Discussions: https://github.com/TDAmeritrade/stumpy/discussions And please describe what it is that you are trying to do? The style file is not needed for STUMPY (it is only there to keep our plots looking somewhat consistent) and you can safely comment/ignore that line out if you are trying to copy the steps in the tutorial or you can save a copy of our style file and place it in the same directory as your notebook. |
@Meng6 Alternatively, you can replace:
with:
And it should work. I've updated all of the tutorials with that now |
Hi @seanlaw, it works! Thanks so much for your help & quick response! |
I updated the fast pattern matching tutorial and welcome any suggestions.
A question from my side: I wrote this some while ago, when there was still an
excl_zone
parameter. I wanted to demonstrate that if settingexcl_zone
to zero inmatch
, you could get the same behavior as when doing the pattern search by hand as in the first part of the tutorial. Now this is no longer possible. Do you have any suggestions on what to do? It feels natural to be able to disable the exclusion zone if necessary, but this would require anignore_trivial
parameter or so that is not there.