Skip to content

Fixed #1006 RuntimeWarning divide by zero encountered in divide - #1012

Open
NimaSarajpoor wants to merge 11 commits into
stumpy-dev:mainfrom
NimaSarajpoor:fix-divide-by-zero-warning
Open

Fixed #1006 RuntimeWarning divide by zero encountered in divide#1012
NimaSarajpoor wants to merge 11 commits into
stumpy-dev:mainfrom
NimaSarajpoor:fix-divide-by-zero-warning

Conversation

@NimaSarajpoor

@NimaSarajpoorNimaSarajpoor commented Jul 21, 2024

Copy link
Copy Markdown
Collaborator

See #1006

This PR addresses part1, 2, and 3 as discussed here: #1006 (comment)

@NimaSarajpoorNimaSarajpoor changed the title [WIP] ] Fixed ##1006 resolve RuntimeWarning divide by zero encountered in divide[WIP] Fixed ##1006 resolve RuntimeWarning divide by zero encountered in divideJul 22, 2024
@NimaSarajpoorNimaSarajpoor changed the title [WIP] Fixed ##1006 resolve RuntimeWarning divide by zero encountered in divide[WIP] Fixed #1006 resolve RuntimeWarning divide by zero encountered in divideJul 22, 2024
@codecov

codecovBot commented Jul 22, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.42%. Comparing base (3077d0d) to head (5e056d3).
Report is 2 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

Comment threadstumpy/core.py Outdated
Comment threadtests/test_core.py Outdated
@NimaSarajpoor
NimaSarajpoorforce-pushed the fix-divide-by-zero-warning branch from aea50d9 to f154e2eCompareJuly 25, 2024 03:42
@NimaSarajpoorNimaSarajpoor changed the title [WIP] Fixed #1006 resolve RuntimeWarning divide by zero encountered in divideFixed RuntimeWarning divide by zero encountered in divide in core.preprocess_diagonalJul 25, 2024
@NimaSarajpoorNimaSarajpoor changed the title Fixed RuntimeWarning divide by zero encountered in divide in core.preprocess_diagonalFixed #1006 RuntimeWarning divide by zero encountered in divideJul 25, 2024

@NimaSarajpoorNimaSarajpoor left a comment

Copy link
Copy Markdown
CollaboratorAuthor

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?

Comment threadstumpy/ostinato.py
distance_profile = core._mass(
Q,
Ts[i],
QT = core.sliding_dot_product(Q, Ts[i])

@NimaSarajpoorNimaSarajpoorJul 26, 2024

Copy link
Copy Markdown
CollaboratorAuthor

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.

Copy link
Copy Markdown
Contributor

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.

@NimaSarajpoorNimaSarajpoorJul 27, 2024

Copy link
Copy Markdown
CollaboratorAuthor

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

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seanlaw

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?

Copy link
Copy Markdown
Contributor

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@NimaSarajpoor@seanlaw