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

Features as functions #400

Merged
merged 22 commits into from Sep 1, 2014

Conversation

Projects
None yet
3 participants
@jabooth
Member

jabooth commented Jul 21, 2014

This PR removes image.features and instead makes the feature functions we already had robust to working with menpo images and numpy arrays. This is achieved through using one of three decorators as appropriate:

@imgfeature
def feature_written_for_menpo_image(image):
   # image will always be a Menpo Image
   # must return a Menpo Image.

@ndfeature
def feature_writen_for_ndarray(pixels):
   # pixels will always be a ndarray of pixels
   # must return an ndarray of pixels

@winitfeature
def feature_that_uses_menpos_windowiterator(pixels):
    # pixels will always be an ndarray.
    # I have to return the feature ndarray and the centres
    # from window iterator. This will be used for
    # correcting masks and landmarks. 

The decorators ensure that each of these feature functions can be used in a consistent manor - if you provide a MaskedImage with landmarks to any of these you will get a MaskedImage back with corrected mask and landmarks - if you provide simply a numpy array to any of these you will get just a numpy array back.

A nice consequence of this design is that any features we expose from Cyvlfeat can be instantly made usable in the fit framework by simply adding the @ndfeature decorator to them.

This part of the work is fairly small, but it has some large consequences for the fitmultilevel module. Currently there is a lot of machinery in fitmulitlevel that deals with checking and applying features. This is massively simplified in this PR, but it does mean a fairly large change set, so we will need to be vigilant for problems.

This PR is not yet ready, but I need to give something at GOSH my attention for a few days so I thought I would put it up so that people can start reviewing. I've listed what I think needs doing at the end of the PR, and will work through these in the next few days.

Hit me with any questions!

TODO

  • Unify feature checking between SDAAM, SDM, AAM
  • Add wrapt to setup.py
  • investigate fitting issues on AAM (constrain_landmarks_to_bounds?)
  • Decide how to handle constrain_landmarks_to_bounds in fitmultilevel
  • Unit tests for new feature decorators
  • Fix Menpo notebooks
  • Documentation of feature API (maybe in user guide?)

Key Changes:

  1. menpo.image.features moved to menpo.features (in keeping with not having deeply nested namespaces)
  2. Features are now functions that take as a first argument an ndarray or menpo Image instance. The return type of the function must match the type of the first argument.
  3. Feature functions have to be decorated with one of three special decorators - @imgfeature, @ndfeature, or @winitfeature. The choice of decorator is made based on the input type of the first argument of the function. Users can choose to write features that take Menpo Image instances - these functions must be decorated with @imgfeature. If users wish they can just work on numpy arrays - in this case, decorate the feature with @ndfeature. Finally, for the special case of functions that work as part of the Cython Window Iterator framework, use the @winitfeature decorator.
  4. After decoration, all features will efficiently work with either numpy arrays or Menpo Image classes. The return type to the user will match whatever type is provided. If an Image is provided, landmarks will be automatically adjusted. If a MaskedImage is provided, it's .mask is automatically corrected by the framework.
  5. Features can no longer have Image specific functionality (such as constrain_landmarks_to_bounds as a kwarg) as all features have to work on plain numpy arrays as well.
  6. Features are performant. Using an @ndfeature on an ndarray only adds a tiny overhead over an undecorated function.
  7. End users are encouraged to make specialised features 'on the fly' - in notebooks for example. These will work just as well in the fit framework as 'native' Menpo-provided features.
  8. Common features are provided for end users at menpo.feature.somefeature.
  9. functools.partial can be used to customise a feature for use in a framework, by providing a configuration of kwargs. For example: sparse_hog = partial(hog, mode='sparse')
  10. img.features.afeature() is removed. Use afeature(img) instead.
  11. The fit framework is considerably simplified in light of these changes. Validation code for features provided to the fit package is unified and simplified.
@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Aug 12, 2014

@jabooth walk me through a brief tutorial of how I can close this PR? Particularly, what's the issues around constrain_landmarks_to_bounds?

size n_rows x n_columns x 2, where window_centres[:, :, 0] are the
row indices and window_centres[:, :, 1] are the column indices.
constrain_landmarks : bool

This comment has been minimized.

@jabooth

jabooth Aug 12, 2014

Member

@patricksnape 2. In the old regime, when you invoke features as a method on an image, we have this kwarg default to true. This means currently in Menpo for most practical cases we 'automatically' constrain mask to landmarks every time we compute a feature.

return new_image
@wrapt.decorator
def imgfeature(wrapped, instance, args, kwargs):

This comment has been minimized.

@jabooth

jabooth Aug 12, 2014

Member

@patricksnape 3. These 3 functions (line 72-110) is the only magic source left for features in this PR. These decorators are placed over functions that work on numpy arrays only. They transform the function into one which will work on Menpo Images and numpy arrays transparently, with a very small overhead. Notice how with this design the arguments/kwargs to any feature should work regardless of whether we are working on a numpy array or Menpo Image - for that reason I removed the (Image specific) constrain_landmarks kwarg.

This comment has been minimized.

@patricksnape

patricksnape Aug 12, 2014

Contributor

Right. So essentially we should move the constrain to a specific call within the model builders?

This comment has been minimized.

@jabooth

jabooth Aug 12, 2014

Member

Yup, see below (sorry on train!)

@jabooth

This comment has been minimized.

Member

jabooth commented Aug 12, 2014

With the above, we have a slight problem - before constrain_landmarks_to_bounds was being implicitly called every time we computed features - now we want to make it explicit. The open question is - should we compute this every time? If so, we should probably make a small wrapper function in fit multilevel that just calls the feature on the image, then constrain landmarks to bounds. This should probably be done in the check features function (after we are happy we have good functions for features, wrap each in the function in a list comprehension and we are done).

@jabooth jabooth added the in progress label Aug 27, 2014

@jabooth jabooth self-assigned this Aug 27, 2014

jabooth added some commits Aug 27, 2014

supress feature_type assets in tests
We currently have a small wrapper around every feature we build that
ensures that constrain_landmarks_to_bounds() is called after feature
computation in the fit framework. This breaks these tests. It may be
desirable in the future to revisit this with a better solution, but
for now this is holding up a huge improvement to Menpo - we can live
with the features being a little less discoverable in the short term.
@jabooth

This comment has been minimized.

Member

jabooth commented Aug 27, 2014

This is now ready, except one SDM test now fails. Waiting for approval from @nontas that this isn't a genuine bug regression. Otherwise, it's ready.

jabooth and others added some commits Aug 28, 2014

@jabooth

This comment has been minimized.

Member

jabooth commented Sep 1, 2014

@nontas @patricksnape did you look into this? Let's get this put to bed!

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Sep 1, 2014

So, I made that one fix. All the notebooks run and the results seem reasonable. Just Nontas needs to check if he's happy with updating the failing SDM test!

@nontas

This comment has been minimized.

Member

nontas commented Sep 1, 2014

Test is fixed, everything seems fine!
+1

jabooth added a commit that referenced this pull request Sep 1, 2014

Merge pull request #400 from jabooth/rmfeatures
Features as functions

@jabooth jabooth merged commit 2ab8014 into menpo:master Sep 1, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@jabooth jabooth deleted the jabooth:rmfeatures branch Sep 1, 2014

@jabooth

This comment has been minimized.

Member

jabooth commented Sep 1, 2014

The difference in the test is due to a difference in behaviour between this PR and master when it comes to applying a no operation feature in the fit framework.

Historically, landmarks were made to be constrained to bounds after the application of a feature. If the feature was None (no feature), then no constraining was done, which is an odd inconsistency.

The new approach on this PR always constrains at the point of feature application, whether the feature is none (no_op) or not. In realising this behaviour, this led to a discussion between @nontas @jalabort and myself about whether we should be 'auto' constraining at all in the fit framework. In the end we decided that we preferred at least the consistency that the new approach provides. We should consider later down the line whether we want to constrain at all or not.

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