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

Channels flip #524

Merged
merged 27 commits into from Feb 18, 2015

Conversation

Projects
None yet
4 participants
@jalabort
Member

jalabort commented Dec 9, 2014

This pull request moves the channel axis in menpo Image (and subclasses) from the very back to the very front. That is, images are now defined to have shape (n_channels, X, Y, ...) instead of (X, Y, ..., n_channels)

  • This change allows for a much faster gradient computation, specially in the case of 2-dimensional multichannel images.
  • The previous should, in turn, lead to faster feature computation (for those features needing to compute gradients) and much faster AAMs in menpofit.

As an example, in my machine, the gradient computation of an old menpo Image of shape (206, 213, 36) took ~ 55ms. The gradient computation of the equivalent menpo Image with the channels at the front (36, 206, 213) takes now ~17ms.

  • Agree on whether the method gradient on image is kept or removed.

Upon discussion: the gradient method will be removed from Image. A new method called set_boundary_pixels will be introduced which can be called after computing the gradient of MaskedImage in order to remove ill-defined boundary pixels along the mask.

  • Adapt IGO, ES and Daisy.

Test were updated due to numerical difference between the fast cython 'gradient' and the numpy 'gradient' which lead to slightly different results for the three previous features.

  • Adapt HOGs, LBPS and Glyph.

Note that all test pass. However, HOGs, LBPs and Glyph are only naively adapted to the new image shapes by moving the channel axis to the back. Ideally, these changes should be done in a separate pull request so that this one can be merged in asap .

jalabort added some commits Dec 8, 2014

Starts flippling the channels.
Most of the work is done, some minor tests still failing.
Finishes flipping channel axis on menpo.image
- Some feature in menpo.image still need to be modified

@jabooth jabooth added the in progress label Dec 9, 2014

jalabort added some commits Dec 10, 2014

Adapts remaining features
- Daisy has been correctly adapted
- HOG, LBP, and Glyph have a temporary fix which consist of flipping back the channel axis
Remove dsift and fast_dsift
This is being handled on a separate PR.

@jalabort jalabort referenced this pull request Dec 16, 2014

Merged

Flip channels #13

5 of 5 tasks complete

jalabort added some commits Dec 19, 2014

@jalabort jalabort self-assigned this Dec 19, 2014

patricksnape added some commits Jan 5, 2015

Update some cython code to handle indices better
On Windows, the default Python 'integer' type is not a long,
but a long long. In general, this means we need to handle either
long or long long. For example, in the normals.pyx code, we use
fused_types to template the different integer types. We were
getting a failed test on windows due to not using fused_types
in the extract_patches method. However, fused_types don't play
well with the buffer interface.

Therefore, to make our code a bit more robust to very large
indices, I've changed from int/long to long long for indices.
This means taking more memory, but allows us to create really
massive images without error. If this is a real problem in terms
of memory then we can investigate proper templating with fused
types at a later date.

Note I have not update LBP and HOG with these changes as these
files need properly updating anyway.

@jalabort jalabort referenced this pull request Jan 5, 2015

Closed

Channel flip #20

@jalabort

This comment has been minimized.

Member

jalabort commented Jan 5, 2015

@patricksnape has found some very nasty bugs on the gradient feature. He will correct them throughout the upcoming days. Until then this cannot be merged in.

Template the gradient method - fix Py_ssize_t
In Python, indices are typedef at compile time to use the
Py_ssize_t type. This was causing compilation issues on Windows.
More specifically, we should make these changes across the other
Cython modules. However, at the moment this commit just fixes
extract_patches (and updates normals.pyx).

I also templated the gradient method so that we can use double
or float transparently. Thus, I added tests and ensured that
the new gradient method is identical to the numpy method
for 2D images. I also fixed a bug on the branch in that method
and updated the numpy method to maintain the correct ordering.
This means I removed the fast2d gradient flag.

@jabooth jabooth modified the milestone: 0.5.0 Jan 14, 2015

patricksnape and others added some commits Feb 3, 2015

Merge remote-tracking branch 'upstream/master' into channels_flip
Conflicts:
	menpo/external/skimage/_daisy.py
	menpo/feature/__init__.py
	menpo/feature/features.py
	menpo/feature/predefined.py
	menpo/image/base.py
	menpo/image/masked.py
	menpo/image/test/image_test.py
Fix viewing for the channel flip
This fixes a merge issue. It also adds a new method that handles
rolling the channel axis to the back.
@jabooth

This comment has been minimized.

Member

jabooth commented Feb 13, 2015

I've merged this in with master and gone through and tidied up the slicing a little (a[2, ...] == a[2]). At this point I'm happy to get this in. There are probably some small bugs but I think we are best getting people using this so we find them before we start thinking about the 0.5.0 release. +1

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Feb 16, 2015

This post on Numpy has some nice explanations of the logic for the channel flip (and our default image ordering)

patricksnape added some commits Feb 16, 2015

Dirty trick for speeding up image loading
ufunc divison is much slower than ufunc multplication, therefore
we can normalise uint8 images using *(1/255) instead of /255,
which is much faster according to my tests.
Fix tests for gradient pinning
Merge went wrong and the tests weren't corrected properly.
Daisy calculating gradient magnitude incorrectly
The new gradient method gives back all the y-gradients, then all
the x-gradients, so we need to account for that in our code.

The previous code was actually BACKWARDS for the arctan2 call.
IGO and ES were wrong from gradient changes
Gradient is now ordered all the ys and then all the xs. Before
it was y and x interleaved. Therefore, we needed to update IGO
and ES to that they calculated correctly. They now return all
the ys and all the xs in order. For double IGO, it goes
all the ys, all the double ys, all the xs, all the double
xs.

@nontas nontas referenced this pull request Feb 17, 2015

Merged

Channels flip #23

@nontas

This comment has been minimized.

Member

nontas commented Feb 17, 2015

After testing this branch (and after the bug fixes done by @patricksnape ) I think this is ready.
+1

Change default graph plotter marker size to 6
Use plot and not scatter so the marker size needs to be smaller.
@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Feb 18, 2015

I'm preferring to be bold when it comes to this PR. I'm going to merge it in this afternoon unless anyone has a good reason not to. +1

@jalabort

This comment has been minimized.

Member

jalabort commented Feb 18, 2015

+1

patricksnape added a commit that referenced this pull request Feb 18, 2015

Merge pull request #524 from jalabort/channels_flip
BREAKING CHANGE: Channels flip

@patricksnape patricksnape merged commit 4c70c0b into menpo:master Feb 18, 2015

2 checks passed

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

@jalabort jalabort deleted the jalabort:channels_flip branch Mar 4, 2015

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