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

Daisy features #472

Merged
merged 4 commits into from Oct 14, 2014

Conversation

Projects
None yet
4 participants
@nontas
Member

nontas commented Oct 10, 2014

This PR adds Daisy features in the menpo.feature package.

It actually replicates the code from scikit-image and changes it so that the features can be extracted from an input image with any number of channels and not only a greyscale image.

It also adds two daisy-related tests.

@jabooth jabooth added the in progress label Oct 10, 2014

@nontas nontas self-assigned this Oct 10, 2014

@nontas nontas added the enhancement label Oct 10, 2014

@jabooth

This comment has been minimized.

Member

jabooth commented Oct 11, 2014

This looks great @nontas. Just one thing - how similar is this code to skimage? If it's adapted from them we need to make sure we include their licence (not a problem as it's compatible with ours).

Scikit-image's licence reads:

 Copyright (C) 2011, the scikit-image team
All rights reserved.

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are
met:

 1. Redistributions of source code must retain the above copyright
    notice, this list of conditions and the following disclaimer.
 2. Redistributions in binary form must reproduce the above copyright
    notice, this list of conditions and the following disclaimer in
    the documentation and/or other materials provided with the
    distribution.
 3. Neither the name of skimage nor the names of its contributors may be
    used to endorse or promote products derived from this software without
    specific prior written permission.

THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
DISCLAIMED. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT,
INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
POSSIBILITY OF SUCH DAMAGE.

I think including the following above the function declaration:

The following function is adapted from scikit-image under the following licence:

Copyright (C) 2011, the scikit-image team
All rights reserved.
...

would suffice. @patricksnape do you agree?

@nontas

This comment has been minimized.

Member

nontas commented Oct 11, 2014

So the code is very similar to the one in skimage (https://github.com/scikit-image/scikit-image/blob/6cac05a924e5abe6d3c2f62efc40e4d93a908522/skimage/feature/_daisy.py). The only difference is that we compute the gradients with menpo.feature.gradient and choose the dominant gradient per pixel in case a multi-channel image is passed in.
I will add the license note as soon as we agree on what it should say.

@nontas

This comment has been minimized.

Member

nontas commented Oct 13, 2014

In moved the main code in menpo.external.skimage._daisy most of which is the same with the corresponding code in skimage. I also changed the default parameter values of menpo.feature.features.daisy so that the output image has 40 channels instead of 200.
I guess the menpo/external/skimage/LICENSE.txt also covers _daisy.py so nothing else needs to be added.

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Oct 14, 2014

Travis is happy, it's in external, I'm happy +1

@jabooth

This comment has been minimized.

Member

jabooth commented Oct 14, 2014

OK, I'm happy, let's just make sure we add that blog post around the release so we can make clear our stance on skimage support in Menpo.

jabooth added a commit that referenced this pull request Oct 14, 2014

@jabooth jabooth merged commit 430115d into menpo:master Oct 14, 2014

2 checks passed

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

@jabooth jabooth removed the in progress label Oct 14, 2014

@jabooth jabooth deleted the nontas:daisy branch Oct 14, 2014

@jalabort

This comment has been minimized.

Member

jalabort commented Oct 15, 2014

Is it possible that this is broken for masked images @nontas ?

@nontas

This comment has been minimized.

Member

nontas commented Oct 15, 2014

Apparently yes... It crashes for me too. I'll try to fix it.

@jabooth

This comment has been minimized.

Member

jabooth commented Oct 15, 2014

Nontas this may be due to the feature API, send me a Hangout if you've got any questions

@nontas

This comment has been minimized.

Member

nontas commented Oct 23, 2014

Btw this is now fixed

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