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

chris-langfield · 2022-02-10T15:50:56Z

Following the discussion in issue #140 we decided to replace the Image.downsample method with a port of the MATLAB algorithm, specifically cryo_downsample. This algorithm uses a square crop in Fourier space. This PR adds a partial port of cryo_crop from the MATLAB code.

It also addresses #554 by moving all functions in aspire.utils.coor_trans to be accessed directly from aspire.utils.

There is a bug in the MATLAB code for padding from size n to n+1 when n is even. Below is the exact logic of the matlab cryo_crop logic for 2D (in Python):

def crop_2d(mat, size, fill_value=0):
    mat_x, mat_y = mat.shape
    start_x = math.floor(mat_x / 2) - math.floor(size / 2)
    start_y = math.floor(mat_y / 2) - math.floor(size / 2)

    # cropping
    if start_x >= 0 and start_y >= 0:
        return mat[start_x : start_x + size, start_y : start_y + size]
    # padding
    elif start_x < 0 and start_y < 0:
        to_return = fill_value * np.ones((size, size))
        to_return[start_x : start_x + size, start_y : start_y + size] = mat
        return to_return

The check of start_x and start_y is supposed to distinguish between cropping and padding. However, an edge case occurs when n is even and we are padding to size n+1. In this case, start_x = start_y = 0, so the cropping code is run. However, because size > mat_x and size > mat_y, we get an index out of bounds error. The implementation in this PR fixes this by slightly modifying the check of start_x and start_y and adding a comparison of the size to mat_x and mat_y.

Reproduce in MATLAB:

inm = diag([1 2 3 4 5 6 7 8 9 10])
out = cryo_crop(inm, 11)

Index in position 1 exceeds array bounds (must not exceed 10).

Error in cryo_crop (line 55)
            mc=m(nsx+1:nsx+n(1),nsy+1:nsy+n(2));

codecov · 2022-02-10T16:01:42Z

Codecov Report

Merging #565 (4f02b16) into develop (b19503c) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #565      +/-   ##
===========================================
+ Coverage    86.48%   86.58%   +0.10%     
===========================================
  Files          108      108              
  Lines         8274     8418     +144     
===========================================
+ Hits          7156     7289     +133     
- Misses        1118     1129      +11

Impacted Files	Coverage Δ
src/aspire/abinitio/commonline_base.py	`92.85% <100.00%> (ø)`
src/aspire/basis/basis_utils.py	`98.17% <100.00%> (ø)`
src/aspire/ctf/ctf_estimator.py	`98.57% <100.00%> (-0.01%)`	⬇️
src/aspire/denoising/adaptive_support.py	`95.65% <100.00%> (ø)`
src/aspire/image/image.py	`89.89% <100.00%> (ø)`
src/aspire/noise/noise.py	`97.50% <100.00%> (-0.84%)`	⬇️
src/aspire/operators/filters.py	`96.29% <100.00%> (ø)`
src/aspire/source/image.py	`96.73% <100.00%> (-0.02%)`	⬇️
src/aspire/source/simulation.py	`99.40% <100.00%> (-0.01%)`	⬇️
src/aspire/utils/__init__.py	`100.00% <100.00%> (ø)`
... and 7 more

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

chris-langfield · 2022-02-11T17:43:30Z

I realized that the old downsample and crop_pad methods exist in aspire/image/preprocess.py still. However, they are only used in test_preprocess.py.

crop_pad in particular has the behavior of crop_2d as a special case, but without the fix described above.

I'm of the opinion that for this targeted change (changing the downsampling implementation), it's better to use this smaller method than try to use/modify aspire.image.preprocess.crop_pad. We can figure out breaking up crop_pad into more specialized utils methods in future work.

src/aspire/utils/coor_trans.py

tests/test_coor_trans.py

j-c-c

Looks great! A couple small things. Double check my logic on the "conditionals" suggestion.

j-c-c

Did a brief sweep of the changes. Looks good to me.

janden

Just some quick comments. Haven't looked at the tests yet.

src/aspire/utils/coor_trans.py

janden

Looks good. Just one small thing.

janden

Thanks!

…o 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

* 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 * initial changes to code and tests * FFB bases * dirac * tentative fixes to mean estimator tests * import * covar3d mat_evaluate fix * unnecessary check * comments * black * fb2d check evaluate * fb3d * ffb3d * fb3d comments * dirac * fb2d * ffb2d * importvolume * pswf * fpswf * class checking logic moved to Basis * asnumpy in evaluate_t fb 3d bases * evaluate_t fixes * f string * fixes * fb2d image check consistent with fb3d * import * ISinstance * an Image * fix Dirac basis evaluate_t * dirac basis comments * remove volume check * Revert "remove volume check" This reverts commit 86d920e. * repr for Volume class * right place to squeeze in estimate() * no pdb * adjusted comments * remove new space clutter * clarified kernel_convolve squeeze * some changes * FB evaluate_t inputs are always Images/Volumes, just take transpose of asnumpy * remove dtype warnings * no pdb * diract evaluate_t take images * move repetitive function tests up to basis_util * cleanups * no self.L pswf * mat_evaluate_t * remove dirac * remove dirac tests * pswf * int size test in _basis_util * polar2d * remove image assert * remove type checks * space * spaces * final format * fix self.size typo * comment in mat_evaluate_t * replace DiracBasis with PolarBasis2D in unit test Co-authored-by: Garrett Wright <47759732+garrettwrong@users.noreply.github.com> Co-authored-by: Garrett Wright <garrettwrong@gmail.com> Co-authored-by: Joakim Andén <janden@kth.se> Co-authored-by: Josh Carmichael <carmichael@princeton.edu> Co-authored-by: Josh Carmichael <66811911+j-c-c@users.noreply.github.com>

* 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 <47759732+garrettwrong@users.noreply.github.com> Co-authored-by: Garrett Wright <garrettwrong@gmail.com> Co-authored-by: Joakim Andén <janden@kth.se> Co-authored-by: Josh Carmichael <carmichael@princeton.edu> Co-authored-by: Josh Carmichael <66811911+j-c-c@users.noreply.github.com>

chris-langfield added 6 commits February 8, 2022 11:23

utils.coor_trans methods accessible from second level

3c1d65a

crop_2d method

04dee02

crop2d test

7144d8c

clarify flooring behavior for centering and add apdding

66f5015

tests finished

c0c2195

isort shenanigans

86f1592

chris-langfield added the enhancement New feature or request label Feb 10, 2022

chris-langfield self-assigned this Feb 10, 2022

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

05a2030

chris-langfield added 2 commits February 10, 2022 11:11

slightly better comment

4152c8b

better behavior for dtype

ae42f7f

chris-langfield changed the title ~~Add aspire.utils.coor_trans.crop_2d~~ Add aspire.utils.coor_trans.crop_2d and move coor_trans objects to second-level API access Feb 10, 2022

This was referenced Feb 10, 2022

Move from hardcoded to logical downsampling tests #566

Closed

Logical rather than hardcoded downsample tests #567

Merged

chris-langfield linked an issue Feb 10, 2022 that may be closed by this pull request

Downsample bug #140

Closed

chris-langfield requested a review from j-c-c February 11, 2022 17:43

j-c-c reviewed Feb 11, 2022

View reviewed changes

src/aspire/utils/coor_trans.py Outdated Show resolved Hide resolved

j-c-c reviewed Feb 11, 2022

View reviewed changes

src/aspire/utils/coor_trans.py Outdated Show resolved Hide resolved

j-c-c reviewed Feb 11, 2022

View reviewed changes

tests/test_coor_trans.py Show resolved Hide resolved

j-c-c requested changes Feb 11, 2022

View reviewed changes

chris-langfield added 8 commits February 11, 2022 17:10

remove mistaken characters

6975459

Merge remote-tracking branch 'origin/develop' into crop_2d

8c51483

fixes after merge

3b5db0b

last ensure removed

6131ec9

rect tests

8498780

padding tests and fill value test

e194a63

better comments

e426e3f

even better comments

2b2cc8f

np.diag

15bf644

chris-langfield requested a review from j-c-c March 4, 2022 15:18

j-c-c previously approved these changes Mar 4, 2022

View reviewed changes

chris-langfield marked this pull request as ready for review March 4, 2022 21:43

chris-langfield requested a review from janden as a code owner March 4, 2022 21:43

janden requested changes Mar 10, 2022

View reviewed changes

crop_pad_2d

bff3e64

chris-langfield dismissed j-c-c’s stale review via bff3e64 March 10, 2022 18:12

chris-langfield requested a review from janden March 10, 2022 18:49

janden requested changes Mar 18, 2022

View reviewed changes

clarifying comment when padding

4f02b16

chris-langfield requested a review from janden March 18, 2022 20:20

janden approved these changes Mar 21, 2022

View reviewed changes

chris-langfield merged commit b000a87 into develop Mar 21, 2022

chris-langfield mentioned this pull request Apr 25, 2022

aspire.utils.coor_trans objects should be accessible from aspire.utils #554

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

chris-langfield commented Feb 10, 2022 •

edited

Loading

Uh oh!

codecov bot commented Feb 10, 2022 •

edited

Loading

Uh oh!

chris-langfield commented Feb 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

j-c-c left a comment

Uh oh!

j-c-c left a comment

Uh oh!

janden left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

janden left a comment

Uh oh!

janden left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

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

Uh oh!

Conversation

chris-langfield commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chris-langfield commented Feb 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

j-c-c left a comment

Choose a reason for hiding this comment

Uh oh!

j-c-c left a comment

Choose a reason for hiding this comment

Uh oh!

janden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

janden left a comment

Choose a reason for hiding this comment

Uh oh!

janden left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

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

chris-langfield commented Feb 10, 2022 •

edited

Loading

codecov bot commented Feb 10, 2022 •

edited

Loading