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

fix freqkernel #207

Merged
merged 13 commits into from
Feb 25, 2021
Merged

fix freqkernel #207

merged 13 commits into from
Feb 25, 2021

Conversation

JeffFessler
Copy link
Contributor

Addresses #206
The substance here is the changed lines with wrapindex and the corresponding added test.
There is also a subtle point that the kernel needs to be supported in [-N/2, N/2-1) so I added checks for that too (handling both even and odd N cases). And some minor tweaking of docstrings.

FWIW, I'm teaching a graduate image processing course where I am trying to translate everything from Matlab to Julia so getting this psf2tof equivalent working is very desirable.

@codecov
Copy link

codecov bot commented Feb 12, 2021

Codecov Report

Merging #207 (e57a5b3) into master (b4f3b7b) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
+ Coverage   91.07%   91.11%   +0.03%     
==========================================
  Files          10       10              
  Lines        1412     1418       +6     
==========================================
+ Hits         1286     1292       +6     
  Misses        126      126              
Impacted Files Coverage Δ
src/ImageFiltering.jl 77.27% <ø> (ø)
src/mapwindow.jl 86.28% <100.00%> (+0.06%) ⬆️
src/utils.jl 88.23% <100.00%> (+0.93%) ⬆️

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 b4f3b7b...e57a5b3. Read the comment docs.

@JeffFessler
Copy link
Contributor Author

Hi @johnnychen94 are you a maintainer here? This PR has been ready for a while.
Anything else I should do?

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. It looks quite good, just a stylistic point (optional) and suggestion for testing your error conditions too.

src/utils.jl Outdated Show resolved Hide resolved
test/basic.jl Show resolved Hide resolved
Co-authored-by: Tim Holy <tim.holy@gmail.com>
@JeffFessler JeffFessler marked this pull request as draft February 24, 2021 18:27
src/utils.jl Outdated Show resolved Hide resolved
@JeffFessler JeffFessler marked this pull request as ready for review February 24, 2021 20:08
@JeffFessler
Copy link
Contributor Author

I just learned about Aqua.jl and tested it out on ImageFiltering.jl and it reported that ImageMetadata is unused so I removed it from deps. This is a bit off topic from freqkernel but figured I'd include it in this PR anyway and see how it goes...

@JeffFessler
Copy link
Contributor Author

Also addressing #208 with a 1-line fix. Hopefully.

@JeffFessler JeffFessler marked this pull request as draft February 25, 2021 02:35
@JeffFessler JeffFessler marked this pull request as ready for review February 25, 2021 03:42
@JeffFessler JeffFessler requested a review from timholy February 25, 2021 03:42
@timholy timholy merged commit 6f0dbc4 into JuliaImages:master Feb 25, 2021
@timholy
Copy link
Member

timholy commented Feb 25, 2021

Very nice work, @JeffFessler. Thanks!

@timholy
Copy link
Member

timholy commented Feb 25, 2021

@johnnychen94
Copy link
Member

johnnychen94 commented Feb 26, 2021

I was AFK for the last two months so sorry for the inactivity. I've looked at the psf2otf source codes two years ago so I'm contaminated and thus can't review this PR (#81). Thank you both for the efforts.

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