Skip to content

Conversation

@NileGraddis
Copy link
Contributor

addresses #660 #659

just a bugfix so master isn't broken :)

@NileGraddis NileGraddis requested review from nicain and njmei May 20, 2019 17:36
@njmei
Copy link
Contributor

njmei commented May 20, 2019

Wouldn't from skimage.transform import resize as imresize be easier and more straightforward?

Copy link
Contributor

@nicain nicain left a comment

Choose a reason for hiding this comment

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

test_lsn_image_to_screen covers lsn_image_to_screen (which your change depends on), but this test is not run on Appveyor/Travis. Can you check test_stimulus_info.py manually, or run a CI branch build?

@NileGraddis
Copy link
Contributor Author

NileGraddis commented May 20, 2019

@njmei It's not quite a drop-in. The comparison is:

target_size = tuple( int(pixels_per_patch * dimsize) for dimsize in img.shape[::-1] )
img_full_res = np.array(Image.fromarray(img).resize(target_size, 0))

vs.

target_size = tuple( int(pixels_per_patch * dimsize) for dimsize in img.shape )
img_full_res = resize(img, target_size, order=0)

The second option is a bit pithier but also uses a different backend (imresize was a thin PIL wrapper). warp also appears to be a bit of a WIP:
https://github.com/scikit-image/scikit-image/blob/bcefaf5d2c2535c440610bb72b2794e1d249951d/skimage/transform/_warps.py#L817

Anyways I think the tradeoff is:

  • improved readability (using resize)
  • low chance of regressions / unexpected behavior (using PIL)

I don't have a strong opinion. Care to vote?

@NileGraddis
Copy link
Contributor Author

NileGraddis commented May 20, 2019

@nicain I ran these tests manually. They pass

@njmei
Copy link
Contributor

njmei commented May 20, 2019

Oh interesting, I wonder why the scipy documentation suggested the skimage resize then...
https://docs.scipy.org/doc/scipy-1.1.0/reference/generated/scipy.misc.imresize.html#scipy.misc.imresize

I'll defer to your judgement!

@nicain nicain self-requested a review May 20, 2019 18:00
Copy link
Contributor

@nicain nicain left a comment

Choose a reason for hiding this comment

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

OK, Looks good to me. @NileGraddis your call on Image vs skimage.resize

@nicain
Copy link
Contributor

nicain commented May 20, 2019

@NileGraddis Since this is part of the 0.16.3 release, can you update the changelog, what's new" section of index.rst, and Rebase the release candidate branch after merging (as per https://github.com/AllenInstitute/AllenSDK/blob/master/RELEASE_INSTRUCTIONS.md)?

Just did this, so don't worry about it.

@codecov-io
Copy link

Codecov Report

Merging #662 into master will decrease coverage by <.01%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #662      +/-   ##
==========================================
- Coverage   44.94%   44.94%   -0.01%     
==========================================
  Files          92       92              
  Lines       11880    11880              
==========================================
- Hits         5340     5339       -1     
- Misses       6540     6541       +1
Impacted Files Coverage Δ
allensdk/core/reference_space.py 93% <ø> (-0.07%) ⬇️
allensdk/brain_observatory/stimulus_info.py 78.58% <20%> (-0.18%) ⬇️

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 966dbf7...4b524ff. Read the comment docs.

1 similar comment
@codecov-io
Copy link

Codecov Report

Merging #662 into master will decrease coverage by <.01%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #662      +/-   ##
==========================================
- Coverage   44.94%   44.94%   -0.01%     
==========================================
  Files          92       92              
  Lines       11880    11880              
==========================================
- Hits         5340     5339       -1     
- Misses       6540     6541       +1
Impacted Files Coverage Δ
allensdk/core/reference_space.py 93% <ø> (-0.07%) ⬇️
allensdk/brain_observatory/stimulus_info.py 78.58% <20%> (-0.18%) ⬇️

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 966dbf7...4b524ff. Read the comment docs.

@nicain nicain merged commit 63f7bb8 into master May 20, 2019
@nicain nicain deleted the 660/remove-imresize branch May 20, 2019 19:39
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.

5 participants