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

remove cyclic target reference from landmarks #395

Merged
merged 6 commits into from Jul 16, 2014

Conversation

Projects
None yet
2 participants
@jabooth
Member

jabooth commented Jul 15, 2014

Following from #394 we identified Landmarks and Image Features as the two places in Menpo where we had circular references. Although weakref sorted out the memory problems, we still have to handle the complexity of holding onto a target in Landmarks - a complexity that we arguably don't gain much from. This PR removes .target from LandmarkManager and LandmarkGroup entirely. As a result:

  • Landmarks are nicely simplified
  • Landmarks can no longer expect to be able to view their target. Where previously we did
foo.landmarks.view()  # view foo and it's landmarks
foo.landmarks[None].lms.view()  # view landmarks on their own as PC

we now more explicitly do

foo.view_landmarks()  # view foo and it's landmarks
foo.landmarks.view()  # view landmarks on their own.

this method is provided by a new mixin, LandmarkableViewable(Landmarkable, Viewable). This is actually a very simple change (only Shape and Image need to be updated to this superclass). We now also gain the ability to customise the logic for this method on a class by class basis, so this means we can fix the multi-chanenel viewer/landmark viewer problem (although I'll leave that for someone else to tackle!

I've fixed all unit tests and everything seems to be working, this could do with some testing though.

TODO

  • Fix documentation for LandmarkableViewable
@property
def landmarks(self):
if self._landmarks is None:
self._landmarks = LandmarkManager(self.n_dims)

This comment has been minimized.

@jabooth

jabooth Jul 15, 2014

Member

Also note that I've moved to lazy instantiation of .landmarks. This should further cheapen the creation of temporary PointCloud and Image instances with the copy=False performance kwarg.

This comment has been minimized.

@jabooth

jabooth Jul 15, 2014

Member

Master:

%timeit Image(pixels, copy=False)
100000 loops, best of 3: 5.63 µs per loop

With lazy landmarks:

%timeit Image(pixels, copy=False)
100000 loops, best of 3: 3.36 µs per loop
@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jul 16, 2014

Ok, looks good! +1

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

Merge pull request #395 from jabooth/rmlmtarget
remove cyclic target reference from landmarks

@jabooth jabooth merged commit 3ece8d0 into menpo:master Jul 16, 2014

1 check passed

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

@jabooth jabooth deleted the jabooth:rmlmtarget branch Jul 16, 2014

@jabooth jabooth referenced this pull request Jul 18, 2014

Merged

Copyable - fast robust copy() throughout Menpo. #398

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment