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

1D Fermat Number Transform deconvolution #14

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

1D Fermat Number Transform deconvolution #14

wants to merge 8 commits into from

Conversation

jakubwro
Copy link
Collaborator

@jakubwro jakubwro commented Jan 5, 2020

I implemented 1D case of FNT deconvolution what partially solves #2 I'll update docs if it makes sense.
The biggest limitation of this method is that each element of H matrix needs to have inverse, what is hard to achieve for real data, so I'm not sure if Deconvolution.jl is good place for algorithms like this.

@jakubwro
Copy link
Collaborator Author

jakubwro commented Jan 5, 2020

Build fails cause this PR is not merged yet JuliaRegistries/General#7482

src/fermat.jl Outdated Show resolved Hide resolved
h = [3, 2, 0, 0]
y = [3, 5, 3, 0]
x = fermat(y, h, 4, 17)
@test Circulant(h) * x == y
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Circular convolution again, it uses the same fft approach as above
https://github.com/JuliaMatrices/SpecialMatrices.jl/blob/master/src/toeplitz.jl#L54

I have feeling that we need some package with better api to perform convolution. I don't like the fact that floating point arithmetic is used here.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like you have a plan 🙂

Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

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

The biggest limitation of this method is that each element of H matrix needs to have inverse, what is hard to achieve for real data, so I'm not sure if Deconvolution.jl is good place for algorithms like this.

While currently implemented methods are probably easier to use in practice, I don't see why this would be a problem for including this method here 😉

h = [3, 2, 0, 0]
y = [3, 5, 3, 0]
x = fermat(y, h, 4, 17)
@test Circulant(h) * x == y
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you have a plan 🙂

src/fermat.jl Outdated Show resolved Hide resolved
src/fermat.jl Outdated Show resolved Hide resolved
xm = ifnt(H_inv .* fnt(convolved, g, q), g, q)
x0 = mod.(Dm * xm, q)
x0[x0 .>= bias] .-= q
x = x0
Copy link
Member

Choose a reason for hiding this comment

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

x will be exactly the same thing as x0, this means that chaning x0 will have the same effect on x and vice versa. Is this what you want?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this assignment is just to follow variable naming form the paper, x0 is not modified later, and new variable xj is introduced later int the loop.

@codecov-io
Copy link

codecov-io commented Jan 6, 2020

Codecov Report

Merging #14 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #14   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      4    +1     
  Lines          25     48   +23     
=====================================
+ Hits           25     48   +23
Impacted Files Coverage Δ
src/Deconvolution.jl 100% <ø> (ø) ⬆️
src/fermat.jl 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7af04b8...43c47b1. Read the comment docs.

@jakubwro
Copy link
Collaborator Author

jakubwro commented Jan 6, 2020

@giordano build is green, but please do not merge yet, I need to add docs and replace fft with precise integer convolution. To do that I think I will start a new project for integer DSP algorithms.

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