-
Notifications
You must be signed in to change notification settings - Fork 332
Fixed #1006 RuntimeWarning divide by zero encountered in divide
#1012
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
base: main
Are you sure you want to change the base?
Fixed #1006 RuntimeWarning divide by zero encountered in divide
#1012
Conversation
divide by zero encountered in divide
divide by zero encountered in divide
divide by zero encountered in divide
divide by zero encountered in divide
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1012 +/- ##
=======================================
Coverage 97.41% 97.42%
=======================================
Files 89 89
Lines 14922 14959 +37
=======================================
+ Hits 14537 14574 +37
Misses 385 385 ☔ View full report in Codecov by Sentry. |
aea50d9
to
f154e2e
Compare
divide by zero encountered in divide
divide by zero encountered in divide
in core.preprocess_diagonal
divide by zero encountered in divide
in core.preprocess_diagonal
divide by zero encountered in divide
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.
@seanlaw
I have one comment. Can you please take a look and let me know what you think?
distance_profile = core._mass( | ||
Q, | ||
Ts[i], | ||
QT = core.sliding_dot_product(Q, Ts[i]) |
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.
Since Q
and Ts[i]
are UNPROCESSED, QT[idx]
can get meaningless value for a non-finite subsequence that starts at idx
. Better to just replace them with np.nan
. So, for example, we can do something like this after computing QT
(QT = core.sliding_dot_product(Q, Ts[i])
):
# post-processing QT
if np.any(~np.isfinite(Q)):
QT[:] = np.nan
QT[~np.isfinite(M_Ts[i])] = np.nan
Note that QT[idx]
will be ignored though eventually once it comes to computing its corresponding value in the distance profile via core.calculate_distance_profile
(see a few lines below). In other words, we do not need the "post-processing of QT" but I think it makes code clearer.
Any concerns?
[Update]
Note: Since we pass inputs with non-finite values to core.calculate_distance_profile
(Not only QT
but also M_T
), maybe I should better stop here and try to address #1011 first. Then come back and resume the work on this PR.
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.
I understand the need to replace the ugly Ts[Ts_idx][subseq_idx : subseq_idx + m]
with Q
. That's fine but
QT = core.sliding_dot_product(Q, Ts[i])
doesn't seem to require any post-processing when either Q
or T
contain np.nan
.
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.
My bad! You are right. No need for post-processing QT
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.
maybe I should better stop here and try to address #1011 first. Then come back and resume the work on this PR.
I did an experiment and shared the result in #1011. We can either work on that and then come back here. Or, on the second thought, we can still merge this (after I resolve the merge conflicts), and then work on #1011.
What do you think?
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.
Since we have a merge conflict here anyways, let's fix #1011 first and then come back here and strikethrough the comment:
Note: Since we pass inputs with non-finite values to core.calculate_distance_profile (Not only QT but also M_T), maybe I should better stop here and try to address #1011 first. Then come back and resume the work on this PR.
so that we are focused on this issue/PR instead of being distracted by jumping back and forth. I am getting confused by having so many things open and so it feels disorganized
See #1006
This PR addresses part1, 2, and 3 as discussed here: #1006 (comment)