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

Groundwork for dense correspondence pipeline #393

Merged
merged 14 commits into from Jul 14, 2014

Conversation

Projects
None yet
3 participants
@jabooth
Member

jabooth commented Jul 9, 2014

This PR contains general improvements and additions to Menpo that are needed for the correspondence pipeline that will be a follow-up PR.

I've decided to split the work as everything presented here is general and useful outside of the scope of the correspondence framework. It should be straightforward to review all this code in isolation and once we are happy it can be brought in.

The correspondence PR actually won't be very large - this really contains the necessary tooling - all the correspondence package will do is string it all together in an easy to use way.

from menpo.visualize import TriMeshViewer
from .normals import compute_normals

This comment has been minimized.

@patricksnape

patricksnape Jul 10, 2014

Contributor

Why is this relative and nothing else is?

This comment has been minimized.

@jabooth

jabooth Jul 10, 2014

Member

Idea is: when importing another asset from within this package (here, menpo.shape) use relative. If you need something from another Menpo package, use absolute.

Motivation being clarity of namespacing. If you have an absolute import you should always respect Menpo's API placement of classes and functions, not their actual full path

from menpo.shape import TexturedTriMesh

not

from menpo.shape.mesh.textured import TexturedTriMesh

This gives users browsing the source code the best chance to not learn bad habits of importing the non-public api.

Of course within a package you may well need functions that aren't exposed from the __init__.py (this being a good example) - the import being relative is a hint that this is internal API that is being imported. Furthermore, you can run into nasty import issues if you rely on the external API placement from within a package.

This is all up for debate but is the current scheme used so this is consistent. Wouldn't be hard to have a PR where we change everything to be consistent one way or another (for instance, we could look at lazy loading a lot more of the code to cut down on initialisation time).

This comment has been minimized.

@patricksnape

patricksnape Jul 10, 2014

Contributor

Then shouldn't it be from . import PointCloud?

This comment has been minimized.

@jabooth

jabooth Jul 10, 2014

Member

Depends if the granularly is at the menpo.shape.mesh package or menpo.shape. I'm a little loathe to start using the up a level relative package .. operator though, which is what we would need to do here right?

class CylindricalUnwrap(Transform):
r"""
Unwraps 3D points into cylindrical coordinates:
x -> radius * theta

This comment has been minimized.

@patricksnape

patricksnape Jul 10, 2014

Contributor

Should this be indented so it appears as a codeblock when turned into docs?

This comment has been minimized.

@jabooth

jabooth Jul 10, 2014

Member

Sorry I should have checked the documentation and fixed this and the below problem. Will do it now.

radius - the distance of the unwrapping from the axis.
z - the distance along the axis of the cylinder (maps onto the y
coordinate exactly)
theta - the angular distance around the cylinder, in radians. Note

This comment has been minimized.

@patricksnape

patricksnape Jul 10, 2014

Contributor

This is an incorrect docstring format.

@jabooth

This comment has been minimized.

Member

jabooth commented Jul 11, 2014

docs now fixed up @patricksnape

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jul 14, 2014

OK, I'm happy with the fixed docs etc +1

return self.n
def _apply(self, x, **kwargs):
return x[:, :self.n].copy()

This comment has been minimized.

@jalabort

jalabort Jul 14, 2014

Member

These two are pretty curious transforms... But I can see they can simplify the code a lot if dimensions need to be added to or removed from shapes.

This comment has been minimized.

@patricksnape

patricksnape Jul 14, 2014

Contributor

I had this argument with @jabooth already haha. He makes a good point that in the case of rasterization, this is necessary, and it allows him to build a single transform that performs his whole pipeline, which is pretty cool!

[0, 0, 1]], dtype=np.float))
# reduce the full transform chain to a single affine matrix
transforms = [rem_z, invert_y, t, unit_scale, im_scale, xy_yx]
return reduce(lambda a, b: a.compose_before(b), transforms)

This comment has been minimized.

@jabooth

jabooth Jul 14, 2014

Member

@patricksnape @jalabort I came here in evidence of why ExtractNDims/AppendNDims are useful, and found I actually didn't use them! Instead I make rem_z, a Homogeneous that does the same thing as ExtractNDims(1). To be honest this is preferable here as it means all the transforms natively compose into a single homogeneous matrix. Although I have used ExtractNDims/AppendNDims in the forthcoming correspondence code, I'm thinking now that maybe we remove those classes and go for this approach instead. It would look something like:

def dims_3to2():
    return Homogeneous(np.array([[1, 0, 0, 0],
                                 [0, 1, 0, 0],
                                 [0, 0, 0, 1]]))

def dims_2to3(x=0):
    return Homogeneous(np.array([[1, 0, 0],
                                 [0, 1, 0],
                                 [0, 0, x],
                                 [0, 0, 1]]))

I would remove the other two classes, and just use these functions here (and in the forthcoming correspondence code). These functions could just live in the correspondence package for now, and we can add them to menpo.transform as and when if another use case crops up for them down the line. Thoughts?

This comment has been minimized.

@jalabort

jalabort Jul 14, 2014

Member

Wow!!! This looks much nicer in my opinion. Also, it should be easy to define function for other types of mappings, besides 3d to 2d and vice-versa.

In the more general case, multiplying shapes with Homogeneous matrices is a pretty easy way of removing and adding points to shapes that I had never thought of...

This comment has been minimized.

@jabooth

jabooth Jul 14, 2014

Member

Haha yeah I remember thinking "if only all these transforms could drop out to one matrix" and then realising it was actually really simple! :)

I agree, it wouldn't be difficult to make this a little more general but I can't think of applications at the mo, so may just follow the good old KISS principle and stick with the two hardcoded funcs for now and revisit if there is a use case in future

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

Merge pull request #393 from jabooth/corresgroundwork
Groundwork for dense correspondence pipeline

@jabooth jabooth merged commit 80b5195 into menpo:master Jul 14, 2014

1 check passed

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

@jabooth jabooth deleted the jabooth:corresgroundwork branch Jul 14, 2014

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