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

promote non-inplace crop methods, crop performance improvements #587

Merged
merged 6 commits into from Jun 1, 2015

Conversation

Projects
None yet
3 participants
@jabooth
Member

jabooth commented May 22, 2015

Addresses #482, and prepares for #583.

Summary of changes:

  1. warp_to_shape() now has a fast path for crops (which are simply when the transform is a Translation, order=0, and no out-of-bounds sampling is needed). Image.crop() and friends now take advantage of this and call warp_to_shape(). This cleans up the image package somewhat, as all our reconstruction logic (handling landmarks, different image types, etc) is now all centralized in the warp_to_{shape,mask}() methods.
  2. crop_to_landmarks_{proportion}() now exist as non-inplace versions of crop_to_landmarks_{proportion}_inplace()
  3. All existing inplace crop methods are present but raise deprecation warnings. There implementation is now slower than the non-inplace varient, as they perform extra work (they basically call the non-inplace versions, and then graft the result back on to self). They will go away in 0.6.x.
  4. MaskedImage.crop_to_true_mask() (which is seemingly an unused method) is now non-inplace.

This is fairly major change, but we already have pretty good coverage on the image package and all tests are passing.

jabooth added some commits May 22, 2015

deprecate crop inplace methods
1. warp_to_shape now has a fast path for crops. crop calls this so all
our logic for rebuilding images in warp_to_{} is reused by cropping
2. crop_to_landmarks_{proportion} are now non-inplace
3. All existing inplace crops are present but raise deprecation warnings
4. MaskedImage.crop_to_true_mask now is not inplace.

@jabooth jabooth added the in progress label May 22, 2015

@jalabort

This comment has been minimized.

Member

jalabort commented May 22, 2015

Looks good! +1

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jun 1, 2015

I'm just going to pull this down and quickly test building an AAM

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jun 1, 2015

OK, seems to work fine for AAMs and CLMs, +1

jabooth added a commit that referenced this pull request Jun 1, 2015

Merge pull request #587 from jabooth/crop_rm_inplace
promote non-inplace crop methods, crop performance improvements

@jabooth jabooth merged commit 27408c8 into menpo:master Jun 1, 2015

3 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jabooth jabooth removed the in progress label Jun 1, 2015

@jabooth jabooth deleted the jabooth:crop_rm_inplace branch Jun 1, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment