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

WIP: BREAKING: Various release fixes #687

Merged
merged 24 commits into from May 10, 2016

Conversation

Projects
None yet
3 participants
@patricksnape
Contributor

patricksnape commented May 4, 2016

This is a lazy PR that changes a load of niggly things that have been outstanding for a while. I want to get them in before 0.7.0. The list of changes is given below:

Changes

  • BREAKING set_boundary_pixels is no longer in place
  • BREAKING normalize_inplace has been deprecated and removed. normalize is now a feature that abstracts out the normalisation logic.
  • BREAKING gaussian_pyramid and pyramid always return copies (before the first image was the original image, not copied)
  • BREAKING constrain_to_landmarks/constrain_to_pointcloud/constrain_mask_to_landmarks are no longer in place
  • set_masked_pixels is deprecated in favor of from_vector
  • set_patches is no longer in place
  • as_unmasked can now be set with an iterable. This makes setting colour backgrounds easier.
  • New method rasterize_landmarks that allows easy image rasterization. By default, MaskedImages are masked with a black background. Use as_unmasked to change the colour/not mask.
  • Add bounds method to images. This is defined as ((0, 0), (height - 1, width - 1)) - the set of indices that are indexable into the image for sampling.
  • Add constrain_to_bounds to PointCloud. Snaps the PointCloud exactly to the bounds given.
  • Deprecate constrain_landmarks_to_bounds
  • BREAKING has_landmarks_outside_bounds is now a method.

TODO

  • Add init_from_pointcloud to images
  • Deprecate centre on PointCloud in favour of centre_of_mass
  • Deprecate normalize methods on image

patricksnape added some commits May 3, 2016

Fix set_boundary_pixels
Docs were incorrect, was not actually returning a copy and so now
it does. This is a breaking change!
Refactor normalize method
Deprecate inplace methods and make normalize a feature. Add more
tests to ensure that everything works as expected.
Gaussian/pyramids return copies
Add tests for pyramid shapes
Add boolean image constrain tests
This is actually already covered by the constrain_mask tests
but at least this ensures that changing the return signature
will not throw an error.
constrain_to_landmarks/pointcloud returns copy
BREAKING CHANGE: No longer in place - now returns a copy.
Deprecate build_mask_around_landmarks
Replace with the more verbose but sensible name of
constrain_mask_to_patches_around_landmarks
Deprecate set_masked_pixels in favour of from_vector
This is equivalent anyway so this method is just sugar that
is confusing.
BREAKING: set_patches is no longer in-place
It now returns a copy of the image.
Allow setting as_unmasked with iterable
Makes it easier to set colour images with colour backgrounds
by passing a list/tuple of floats for example
Add rasterize_landmarks
Exposes the rasterization method
Add bounds() method to image
The bounds are defined by ((0, 0), (height - 1), (width - 1)).
This is so that constrain_landmarks_to_bounds can use
these bounds. This makes sense when you consider the bounds as
being inclusive of the set of integer indices it is possible to
index into the image with.
BREAKING: Add the constrain_to_bounds method to pcloud
Deprecate the constrain_landmarks_to_bounds method.
Change has_landmarks_outside_bounds to a method
@@ -768,6 +768,58 @@ def lbp(pixels, radius=None, samples=None, mapping_type='riu2',
@ndfeature
def normalize(pixels, scale_func=None, mode='all'):
r"""
Normalize the pixel values via mean centring and an optional scaling. By

This comment has been minimized.

@nontas

nontas May 4, 2016

Member

spelling mistake: centering

dimension is interpreted as channels. This means an N-dimensional image
is represented by an N+1 dimensional array.
scale_func : `callable`, optional
Compute the scaling factor. Expects a single parameter and any number

This comment has been minimized.

@nontas

nontas May 4, 2016

Member

What is the single parameter? Is it axis?

This comment has been minimized.

@jabooth

jabooth May 9, 2016

Member

No, the single arg is the pixel array right? Although I'm a little hazy on why this needs to be a callable (over simply a keyword for the scale factor directly)..is there a motivating example?

This comment has been minimized.

@jabooth

jabooth May 9, 2016

Member

NVM, just seen the np.norm use case.

def bounds(self):
r"""
The bounds of the image, minimum is always (0, 0). The maximum is
the maximum **index** that can used to index into the image for each

This comment has been minimized.

@nontas

nontas May 4, 2016

Member

spelling: that can be used

@nontas

This comment has been minimized.

Member

nontas commented May 4, 2016

Great job! My only objection is that I would expect set_patches (or any setter function) to be inplace.

patricksnape added some commits May 5, 2016

Add init_from_pointcloud
Needed overloads for each of the image types - was easier
just to duplicate the very small amount of code than try and
make some hierarchy of functions for the sake of 3 lines.

@patricksnape patricksnape changed the title from BREAKING: Various release fixes to WIP: BREAKING: Various release fixes May 6, 2016

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented May 6, 2016

@jabooth Functions like rotate_about_centre require the object to have a centre method - removing this from PointCloud would prevent you from having that single API. So we would either have to do some type inspection or you wouldn't be able to use the method to rotate PointClouds. Maybe we should just leave the whole centre_of_mass thing for now - it is explained in the docs to be fair?

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented May 9, 2016

Appveyor is just being annoying and everything is actually fine.

group : `str`, optional
If ``None``, the pointcloud will only be used to create the image.
If a `str` then the pointcloud will be attached as a landmark
group to the image, with the given string as key.

This comment has been minimized.

@jabooth

jabooth May 9, 2016

Member

landmark attachment very nice addition, definitely useful for most calling cases for this constructor.

This is perhaps another case where it would be nice to have access to the underlying transform that was used as well, a-la
https://github.com/menpo/menpo/blob/master/menpo/image/base.py#L1817

Do we do the same and add a return_transform=False kwarg? Little gross as it's on a pseudo-constructor, but potentially useful and makes it consistent with other image APIs.

This comment has been minimized.

@patricksnape

patricksnape May 9, 2016

Contributor

That is actually quite a good point - I can get behind that.

Edit: Done

Although the *actual* maximum subpixel value would be something
like ``self.height - eps`` where ``eps`` is some value arbitrarily
close to 0, this value at least allows sampling without worrying about
floating point error.

This comment has been minimized.

@jabooth

jabooth May 9, 2016

Member

great explanation

"normalized")
else:
self._from_vector_inplace(centered_pixels / scale_factor)
from menpo.feature import normalize

This comment has been minimized.

@jabooth

jabooth May 9, 2016

Member

we have normaliSe and normaliZe methods now, care to...normali{s,z}e then? ;)

This comment has been minimized.

@jabooth

jabooth May 9, 2016

Member

To be clear:

  • normalise_pixels_range
  • normalize_std
  • normalize_norm

Also, are we going to continue to support these as methods on Image in addition to their role as features? I think I'm right in saying we removed Image.gradient() in favour of gradient(img). Don't have too strong feelings on this, other than that we should be consistent.

This comment has been minimized.

@patricksnape

patricksnape May 9, 2016

Contributor

This is done.

@jabooth

This comment has been minimized.

Member

jabooth commented May 9, 2016

@patricksnape euuurgh yeah that would be grim to have to step away from. For now let's leave it as .centre documented as COM, we'll see if this really needs addressing any deeper later on.

@jabooth

This comment has been minimized.

Member

jabooth commented May 9, 2016

@patricksnape checked over everything, just those few issues commented on to resolve then we are good to go. Nice work!

patricksnape added some commits May 9, 2016

Deprecate normalize methods on image
Add other normalize methods to features including norm, std and
var.
@jabooth

This comment has been minimized.

Member

jabooth commented May 10, 2016

Looks great, +1

Make normalize an image feature
now when masked image normalize is deprecated we can actually rely
on the feature
@patricksnape

This comment has been minimized.

Contributor

patricksnape commented May 10, 2016

OK - I think this is all ok at the moment - going to merge.

@patricksnape patricksnape merged commit 6240681 into menpo:master May 10, 2016

3 of 4 checks passed

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

@patricksnape patricksnape deleted the patricksnape:various_release_fixes branch May 10, 2016

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