Skip to content

Conversation

@chris-langfield
Copy link
Collaborator

@chris-langfield chris-langfield commented Apr 5, 2022

The final stage of adding the new downsampling method.

This relates to issues

#140, #608, #566

The method uses the new crop_pad_2d method in the frequency domain and is tested by new downsampling tests that do not rely on hardcoded results

@codecov
Copy link

codecov bot commented Apr 7, 2022

Codecov Report

Merging #606 (f559ecb) into develop (2806e3d) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #606      +/-   ##
===========================================
- Coverage    86.53%   86.52%   -0.02%     
===========================================
  Files          109      109              
  Lines         8296     8287       -9     
===========================================
- Hits          7179     7170       -9     
  Misses        1117     1117              
Impacted Files Coverage Δ
src/aspire/image/image.py 89.38% <100.00%> (-0.51%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@chris-langfield chris-langfield self-assigned this Apr 25, 2022
@chris-langfield chris-langfield added the enhancement New feature or request label Apr 25, 2022
@chris-langfield chris-langfield mentioned this pull request Apr 25, 2022
@chris-langfield
Copy link
Collaborator Author

Using centered_fft2 fixes the issues from #608. The new downsampling method now passes the logical tests introduced in #567

It is worth noting that Volume.downsample() is not currently replaced with this new method.
It uses the related but not identical method aspire.image.downsample().

Although this method uses a similar strategy, I believe there is a conversation to be had about how to best go about vetting and possibly improving this 3D downsampling method. (I made an issue: #617)

@chris-langfield chris-langfield requested a review from j-c-c April 25, 2022 17:40
Copy link
Collaborator

@j-c-c j-c-c 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! Couple small things.


return Image(im_ds)
# compute FT with centered 0-frequency
fx = np.array([fft.centered_fft2(self.data[i]) for i in range(self.n_images)])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be able to just do fx = fft.centered_fft2(self.data). I don't think the loop is necessary. Same below for ifft.

Comment on lines 217 to 218
np.array([fft.centered_ifft2(crop_fx[i]) for i in range(self.n_images)])
) * (ds_res**2 / self.res**2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just out = np.real(fft.centered_ifft2(crop_fx)) * (ds_res**2 / self.res**2)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer the current set up (the return Image(out) echoes the pattern used in other parts of the code)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was just in reference to the unnecessary list comprehension. Agree with matching other parts of the code for returning an Image.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh totally misunderstood. Your comment is absolutely right and I made the change thanks!

Copy link
Collaborator

@j-c-c j-c-c 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! Couple small things.

@chris-langfield chris-langfield requested a review from j-c-c April 26, 2022 12:27
Copy link
Collaborator

@j-c-c j-c-c 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!

@chris-langfield chris-langfield marked this pull request as ready for review April 26, 2022 13:31
@chris-langfield chris-langfield requested a review from janden as a code owner April 26, 2022 13:31
Copy link
Collaborator

@janden janden 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.

@chris-langfield chris-langfield merged commit 10f613d into develop Apr 28, 2022
@chris-langfield chris-langfield deleted the new_downsample branch April 28, 2022 12:49
garrettwrong pushed a commit that referenced this pull request May 27, 2022
* dropped in new ds method

* adjust tests temporarily

* some imports

* change to centered_fft2 and allow gridpoint checks

* format

* remove debug show()

* comment

* simplify centered_fft2 and centered_ifft2
@chris-langfield chris-langfield mentioned this pull request Jul 6, 2022
chris-langfield added a commit that referenced this pull request Sep 1, 2022
* Fix `k` bug in BFSReddyChetterji

* restrict corr to a disc

* Change `self.L` and `self.n` to `self.src.L` and `self.src.n` for estimators (#550)

* changed self.L and self.n to self.src.L and self.src.n for estimators

* extraneous comma

* Add `aspire.utils.coor_trans.crop_2d` and move `coor_trans` objects to second-level API access (#565)

* utils.coor_trans methods accessible from second level

* crop_2d method

* crop2d test

* clarify flooring behavior for centering and add apdding

* tests finished

* isort shenanigans

* fix even to odd n->n+1 padding bug

* slightly better comment

* better behavior for dtype

* remove mistaken characters

* fixes after merge

* last ensure removed

* rect tests

* padding tests and fill value test

* better comments

* even better comments

* np.diag

* crop_pad_2d

* clarifying comment when padding

* Logical rather than hardcoded downsample tests (#567)

* downsample tests

* cleanup and deleting test downsample from preprocess_pipeline

* remove hardcoded ds / whiten test

* flake8

* 3d volume downsample test

* remove redundant test from tests/test_preprocess.py

* remove unused import

* remove numbered tests for uniformity

* typo

* skip one of the ds tests

* use gaussian_3d to generate test volume

* isort

* checkCenterPoint

* max_resolution -> L_ds

* parametrize ds tests

* Bessel zero finding cleanup (#596)

* docstring for besselj_zeros

* rest of docstrings

* remove pdb import

* second order differences comment

* comments and docstring for num_besselj_zeros

* formatting

* tiny fixes;

* docstring fixes

* NumPy

* NumPy again

* sanity check

* parentheses inside sentence

* `Basis` size fixes (#598)

* fix size checking bug

* fb2d

* fb3d and pswf2d

* dirac

* fpswf

* polar

* format

* sz -> size

* test init with int

* docstring

* typo

* the same typo

* Authors page grammar fix.

* cmap=gray

* Gallery verb tense. Change cmap to gray.

* Update gallery/experiments/README.rst header language.

* Encode size in 2D FB test

* Add test for 2D FB Gaussian expansion

* Add eval–expand test for 2D FB

* Add adjoint test for 2D FB

* Add test for isotropic and modulated in 2D FB

* Add test for indices

* Add test for specific FB 2D basis element

* Make 2D FB tests FFB friendly

Cast `Images` to arrays when needed.

* Move tests not specific to FB into a mixin

* Add mixin to FFB 2D tests

* Remove hardcoding from complex conversion tests

* First attempt at testElement for FFB2D

* Remove hardcoded (F)FB2D tests

* Parameterize tests

* Pass dtype to gaussian_2d

* Expand on comment regarding factor of 1/2

* Fix rtoc bug

* Add shape checks

To make sure we don't get the same inadvertent broadcasting bug again.

* Loop over different indices in 2D (F)FB tests

* Fix #604 (#607)

* Bump version: 0.9.0 → 0.9.1

* Fourier-Bessel Mixin class (#599)

Fourier Bessel Mixin Class: fb.py

* New downsample method (#606)

* dropped in new ds method

* adjust tests temporarily

* some imports

* change to centered_fft2 and allow gridpoint checks

* format

* remove debug show()

* comment

* simplify centered_fft2 and centered_ifft2

* dict of dicts

* adjusted tests

* Zenodo file with authors and affiliations (#615)

* add zenodo file with just creators

* name

* add zenodo file to manifest

* json errors

* chris orcid

* updated affiliations

* update tox.ini to include json.tool check

* josh orcid

* author ordering

* review changes

* Remove offending sk import

* Explicitly use svd_solver='full' for repro unit test

* Fix PIL deprecation warning

* sensible defaults

* optional saving mrc

* populate_unique_filters and read_ctf_file

* body of populate_unique_filters

* loop var

* tests

* work around ImageSource.set_metadata

* update test

* final test

* rationalize sample CTF values

* readme space

* correct metadata test

* write_all_star

* remove write_one_star

* method name

* reading from Relion micrograph_ctf.star

* format

* import_ctf() instead of __init__ option

* added tests and clarified import_ctf method

* condense ctf args coordinate source

* .values() instead of items()

* add save image flags to increase test converage

* docstrings

* rest of docstrings

* rename fun to populate_ctf

* first pass refactor

* refactor 2

* docstring

* refactor 3

Co-authored-by: Garrett Wright <[email protected]>
Co-authored-by: Garrett Wright <[email protected]>
Co-authored-by: Joakim Andén <[email protected]>
Co-authored-by: Josh Carmichael <[email protected]>
Co-authored-by: Josh Carmichael <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants