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 a problem with czt which causes wrap-around. This had an effect… #31

Merged
merged 13 commits into from
Feb 8, 2023

Conversation

RainerHeintzmann
Copy link
Member

fixed a problem with czt which causes wrap-around. This had an effect on resample_czt.
There was also a problem with premature clipping when zooming but also extending the destination size.
See this examples:

img = Float32.(testimage("resolution_test_512.tif")); resample_czt(img, (0.8,1.6), new_size=(1024,1040))

@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2023

Codecov Report

Merging #31 (50f9906) into main (87352d2) will decrease coverage by 0.34%.
The diff coverage is 80.76%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #31      +/-   ##
==========================================
- Coverage   93.34%   93.01%   -0.34%     
==========================================
  Files          17       17              
  Lines         947      959      +12     
==========================================
+ Hits          884      892       +8     
- Misses         63       67       +4     
Impacted Files Coverage Δ
src/resampling.jl 90.16% <72.72%> (-1.44%) ⬇️
src/czt.jl 96.07% <86.66%> (-3.93%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

src/czt.jl Outdated Show resolved Hide resolved
src/resampling.jl Outdated Show resolved Hide resolved
src/resampling.jl Show resolved Hide resolved
docs/src/czt.md Show resolved Hide resolved
src/czt.jl Outdated Show resolved Hide resolved
src/czt.jl Outdated Show resolved Hide resolved
src/resampling (conflicted copy 2023-02-03 080831).jl Outdated Show resolved Hide resolved
src/resampling.jl Outdated Show resolved Hide resolved
src/resampling (conflicted copy 2023-02-03 080831).jl Outdated Show resolved Hide resolved
src/resampling (conflicted copy 2023-02-03 080831).jl Outdated Show resolved Hide resolved
src/resampling (conflicted copy 2023-02-03 080831).jl Outdated Show resolved Hide resolved
docs/src/czt.md Outdated Show resolved Hide resolved
docs/src/resampling.md Outdated Show resolved Hide resolved
src/resampling.jl Show resolved Hide resolved
src/resampling.jl Outdated Show resolved Hide resolved
FourierTools.upsample2
FourierTools.upsample2_abs2
FourierTools.upsample2_1D
FourierTools.barrel_pin
```
Copy link
Member

@roflmaostc roflmaostc Feb 7, 2023

Choose a reason for hiding this comment

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

I'm actually against exporting all those functions. Some of them are rather internally (such as resample_by_RFFT).

So I would only export upsample2 and resample

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not "barrel_pin"? This is not internal.

Copy link
Member

Choose a reason for hiding this comment

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

Ok for barrel_pin.

But rename it to something more expressive.

Copy link
Member Author

Choose a reason for hiding this comment

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

I left barrel_pin in for export

Copy link
Member Author

Choose a reason for hiding this comment

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

backward compatibility? What is wrong with the name?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I was confused why GitHub indicated change of these lines.
Then we leave it but nevertheless.

How should one know that pin stands to pincushion and not for to pin.

Mainly it's a distortion, so something like: distort_radial would be more expressive
https://en.wikipedia.org/wiki/Distortion_(optics)#Radial_distortion

Copy link
Member Author

Choose a reason for hiding this comment

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

we should leave it as is. 'pincushion' is described in the help and I find the name OK. Also we should not change it due to backwards compatibility issues.

src/czt.jl Outdated Show resolved Hide resolved
src/czt.jl Outdated Show resolved Hide resolved
@roflmaostc
Copy link
Member

As I said before, please always check the docstrings for proper formatting in the REPL.
Also check your markdown syntax please. Fixed a couple of mistakes.

src/czt.jl Show resolved Hide resolved
test/czt.jl Show resolved Hide resolved
@roflmaostc roflmaostc merged commit 7a37b89 into main Feb 8, 2023
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