Skip to content
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

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

NimaSarajpoor
Copy link
Collaborator

@NimaSarajpoor NimaSarajpoor commented Jul 21, 2024

See #1006

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

@NimaSarajpoor NimaSarajpoor changed the title [WIP] ] Fixed ##1006 resolve RuntimeWarning divide by zero encountered in divide [WIP] Fixed ##1006 resolve RuntimeWarning divide by zero encountered in divide Jul 22, 2024
@NimaSarajpoor NimaSarajpoor changed the title [WIP] Fixed ##1006 resolve RuntimeWarning divide by zero encountered in divide [WIP] Fixed #1006 resolve RuntimeWarning divide by zero encountered in divide Jul 22, 2024
Copy link

codecov bot commented Jul 22, 2024

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.

stumpy/core.py Outdated Show resolved Hide resolved
tests/test_core.py Outdated Show resolved Hide resolved
@NimaSarajpoor NimaSarajpoor changed the title [WIP] Fixed #1006 resolve RuntimeWarning divide by zero encountered in divide Fixed RuntimeWarning divide by zero encountered in divide in core.preprocess_diagonal Jul 25, 2024
@NimaSarajpoor NimaSarajpoor changed the title Fixed RuntimeWarning divide by zero encountered in divide in core.preprocess_diagonal Fixed #1006 RuntimeWarning divide by zero encountered in divide Jul 25, 2024
Copy link
Collaborator Author

@NimaSarajpoor NimaSarajpoor left a 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])
Copy link
Collaborator Author

@NimaSarajpoor NimaSarajpoor Jul 26, 2024

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
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.

Copy link
Collaborator Author

@NimaSarajpoor NimaSarajpoor Jul 27, 2024

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
Collaborator Author

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
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 free to 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