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

Correlation via fft implementation #2203

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

Conversation

AlexanderKalistratov
Copy link
Collaborator

@AlexanderKalistratov AlexanderKalistratov commented Nov 27, 2024

  • Added keyword method to correlate function similar to scipy correlate
  • If method == 'auto' method is choosing automatically between direct and fft
  • Added implementation of fft-based correlation
  • fft-based implementation may have accuracy issues, so it is validated in non-standard way.

Depends on: #2180, #2202

Copy link
Contributor

View rendered docs @ https://intelpython.github.io/dpnp/pull/2203/index.html

@@ -478,17 +482,57 @@ def _get_padding(a_size, v_size, mode):
return l_pad, r_pad


def _run_native_sliding_dot_product1d(a, v, l_pad, r_pad):
def _choose_conv_method(a, v, rdtype):
assert a.size >= v.size
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it is better to do:

Suggested change
assert a.size >= v.size
if a.size < v.size:
raise ValueError("a.size must not be smaller than v.size")

Comment on lines +554 to +532
assert a.size >= v.size
assert l_pad < v.size
Copy link
Contributor

Choose a reason for hiding this comment

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

These assertions should be turned to checks that raise exceptions if unmet.

This is because Python interpreter allows to turn off assertion checks with -O option:

(dev_dpctl) opavlyk@opavlyk-mobl:~/repos/dpctl$ python -O -c "assert False"
(dev_dpctl) opavlyk@opavlyk-mobl:~/repos/dpctl$ python  -c "assert False"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
AssertionError

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are an internal functions which are supposed to be called from already validated context.
These asserts are more reminders than an actual checks

I could convert them into checks but this would also decrease coverage statistics


# +1 is needed to avoid circular convolution
padded_size = a.size + r_pad + 1
fft_size = 2 ** math.ceil(math.log2(padded_size))
Copy link
Contributor

Choose a reason for hiding this comment

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

Since math.ceil returns float, it should be converted to integer:

Suggested change
fft_size = 2 ** math.ceil(math.log2(padded_size))
fft_size = 2 ** int(math.ceil(math.log2(padded_size)))

@AlexanderKalistratov AlexanderKalistratov force-pushed the correlate branch 2 times, most recently from 936870b to be09e32 Compare December 7, 2024 16:44
Base automatically changed from correlate to master December 9, 2024 20:56
@coveralls
Copy link
Collaborator

Coverage Status

coverage: 65.17% (+0.09%) from 65.08%
when pulling f7df1ac on correlate_fft
into 4d1a6a9 on master.

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.

3 participants