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#938: Speed up njit-decorated function for sliding dot product [WIP] #939

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

Conversation

NimaSarajpoor
Copy link
Collaborator

This PR addresses the issue raised in #938. We want to create a new function for sliding_dot_product that:
(1) has a performance that is close to the performance of core.sliding_dot_product
(2) can be called by a njit-decorated function

Currently, the existing alternative to core.sliding_dot_product is core._sliding_dot_product; however, in contrast to core.sliding_dot_product, its performance is NOT independent of m, the length of query.

As suggested by @seanlaw, we are going to try OTFFT to implement fft and ifft. Then, we use them to create a new function for sliding_dot_product.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

codecov bot commented Dec 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (857063c) 98.93% compared to head (2c5dd4d) 98.93%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #939   +/-   ##
=======================================
  Coverage   98.93%   98.93%           
=======================================
  Files          84       84           
  Lines       14292    14292           
=======================================
  Hits        14140    14140           
  Misses        152      152           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NimaSarajpoor
Copy link
Collaborator Author

@seanlaw
I think the previous notebook got too messy and there were different versions there. I did not remove it (I just kept it for our future reference). Instead, I just pushed a new notebook (see c2a9b43). My goal is to add functions step by step based on the discussions we have had so far on this PR and #938.

For now I just resolved the previously- provided comments (by you and me) that were regarding the initial notebook. I will consider them in this new notebook though.

@@ -0,0 +1,178 @@
{
Copy link
Collaborator Author

@NimaSarajpoor NimaSarajpoor Jan 1, 2024

Choose a reason for hiding this comment

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

Line #20.        x[:] = scipy.fft.fft(x)  # we will implement our fft shortly!

We will later work on implementing this fft function using 6-step / 8-step algorithm. We then change this line and the caller function accordingly.


Reply via ReviewNB

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