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.

Sign up for GitHub

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

Jump to bottom

ctf_freq shape mismatch #40

Merged
merged 1 commit into from
Dec 14, 2021
Merged

ctf_freq shape mismatch #40

merged 1 commit into from
Dec 14, 2021

Conversation

geoffwoollard
Copy link
Contributor

@geoffwoollard geoffwoollard commented Dec 14, 2021 •
edited
Loading

There was a bug that the test was catching that only came up for n_pixels=98, because of how the end point is calculated using np.arange. I've fixed and updated with documentation

Note that freq_pix_1d_safe ensures that we have
    [0,1/(2*n_pixels), 2/(2*n_pixels), ..., (n_pixels - 1) / (2*n_pixels)]
    otherwise np.arange gives an extra point at +0.5 for n_pixels=98
    instead of finishing at
    (n_pixels - 1) / (2*n_pixels) = (49-1)/(2*49) = 48/98 = 0.48979... < 0.5

@geoffwoollard
fixed. the issue was that np.arange gives the end point for n_pixels=…
905571e 
……98, and should be one point less. the spacing is fine, so I just get up to the right index. this is passing: for n_pixels in range(2,2000,2): assert transfer.ctf_freqs(n_pixels,dim=2)[0].shape[0] == n_pixels
@geoffwoollard geoffwoollard changed the title ctf_freq mismatch ctf_freq shape mismatch Dec 14, 2021
@geoffwoollard
Copy link
Contributor Author

geoffwoollard commented Dec 14, 2021 •
edited
Loading

I'm not sure if the main branch is clean... or if the testing environment may not work for other reasons outside of the bug.

If the main branch is not clean you can see the tests pass here geoffwoollard#1

@geoffwoollard geoffwoollard mentioned this pull request Dec 14, 2021
Closed
@codecov
Copy link

codecov bot commented Dec 14, 2021

Codecov Report

Merging #40 (905571e) into master (61babbc) will increase coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   97.15%   97.34%   +0.20%     
==========================================
  Files           5        6       +1     
  Lines         140      150      +10     
==========================================
+ Hits          136      146      +10     
  Misses          4        4
Impacted Files Coverage Δ
simSPI/linear_simulator/noise_utils.py 100.00% <100.00%> (ø)
simSPI/linear_simulator/shift_utils.py 100.00% <100.00%> (ø)
simSPI/transfer.py 94.24% <100.00%> (+0.24%) ⬆️

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 9e389c8...905571e. Read the comment docs.

@geoffwoollard geoffwoollard mentioned this pull request Dec 14, 2021
Closed
@geoffwoollard geoffwoollard added bug Something isn't working documentation Improvements or additions to documentation labels Dec 14, 2021
@fredericpoitevin fredericpoitevin linked an issue Dec 14, 2021 that may be closed by this pull request
test_eval_ctf fails for python 3.7 #37
Closed
fredericpoitevin
fredericpoitevin approved these changes Dec 14, 2021
View reviewed changes
Copy link
Member

@fredericpoitevin fredericpoitevin left a comment

Choose a reason for hiding this comment

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

Looks good to me!

simSPI/transfer.py
@@ -36,12 +41,14 @@ def ctf_freqs(n_pixels, psize=1.0, dim=2):
"""
if dim == 1:
freq_pix_1d = np.arange(0, 0.5, 1 / n_pixels)
freq_1d = freq_pix_1d * psize
freq_pix_1d_safe = freq_pix_1d[: n_pixels // 2]
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it's solving a different error: that line was forgotten initially, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally freq_pix_1d was the right size of n_pixels // 2, but then for size 98 it gets this extra element because of how np.array treats things (closed vs open intervals I suppose). So I just enforce that it doesn't have this extra point. The intervals are all good, before it just sometimes had an extra end point.

simSPI/transfer.py
return freq_1d

# assert d == 2
freq_pix_1d = np.arange(-0.5, 0.5, 1 / n_pixels)
x, y = np.meshgrid(freq_pix_1d, freq_pix_1d)
freq_pix_1d_safe = freq_pix_1d[:n_pixels]
Copy link
Member

Choose a reason for hiding this comment

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

Good catch @geoffwoollard ! I'm still not 100% sure I follow but it seems to do the trick!

@geoffwoollard geoffwoollard merged commit 84df5c5 into compSPI:master Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredericpoitevin fredericpoitevin fredericpoitevin approved these changes

@harshit-gupta-cor harshit-gupta-cor Awaiting requested review from harshit-gupta-cor

Assignees
No one assigned
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test_eval_ctf fails for python 3.7
2 participants
@geoffwoollard @fredericpoitevin