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

Landmarks fixes #402

Merged
merged 7 commits into from Jul 23, 2014

Conversation

Projects
None yet
3 participants
@patricksnape
Contributor

patricksnape commented Jul 23, 2014

Just some updates that help ease the transition for the changes that @jabooth has been making recently in order to break the cyclic/weak references!

  1. Change the indexing of landmark group so that it returns a PointCloud (instead of having to use lms)

    pcloud = img.landmarks['group'].lms  #  The whole PointCloud as before
    pcloud = img.landmarks['group']['label']  # A subset of the original PointCloud
    
  2. Remove the group label from landmark group since we didn't use it anyway

  3. Update viewing to be more useful
    a. @jabooth already added the view_landmarks method. It now has more kwargs that allow you to filter
    the landmarks that you are viewing. So now, we do:
    img.view_landmarks(group_label='ibug_68_points', with_labels=['reye', 'leye'])

You can continue to view landmarks without the underlying object, and they can be filtered in the same way. This just seems more natural and not as cumbersome as it was in master at the moment!

patricksnape and others added some commits Jul 23, 2014

Change landmark group indexing
Now when you index into a landmark group it returns the sliced
pointcloud instead of a new landmark group. This removes
a lot of spurious lms calls throughout the code.
Remove group label from landmark group
This wasn't being handled properly and actually doesn't make much
sense. This group label was basically caching the key into
the landmark manager which it now no longer does!
Add the ability to only view specific groups/labels
This makes viewing landmarks easier with the new layout.
Now you can pass in group_label, without_labels and with_labels.
Update base.py
Super minor doc change
@@ -290,17 +340,13 @@ class LandmarkGroup(Copyable, Viewable):
----------
target : :map:`Landmarkable`
The parent object of this landmark group.

This comment has been minimized.

@jabooth

jabooth Jul 23, 2014

Member

had we not standardised on spacing between doc args or am I wrong?

@jabooth

This comment has been minimized.

Member

jabooth commented Jul 23, 2014

happy to take this as is, docs are not uniform with spacing atm anyway so can standardise that across all of Menpo at a later date. +1

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

@jabooth jabooth merged commit ba86f7e into menpo:master Jul 23, 2014

1 check passed

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

@jabooth jabooth deleted the patricksnape:landmarks_fixes branch Jul 23, 2014

@jalabort

This comment has been minimized.

Member

jalabort commented on menpo/landmark/base.py in e9c5623 Aug 13, 2014

wouldn't this be more useful skipping this if? Maybe I'm missing something about the behaviour of this render_labels...

if with_labels is not None and without_labels is not None:
    raise ValueError('You may only pass one of `with_labels` or '
                               '`without_labels`.')
elif with_labels is not None:
    lmark_group = self.with_labels(with_labels)
elif without_labels is not None:
    lmark_group = self.without_labels(without_labels)
else:
    lmark_group = self  # Fall through

This comment has been minimized.

Contributor

patricksnape replied Aug 14, 2014

with_labels and without_labels are like filters for which labels get displayed. So if you pass both, you end up with some sort of set difference (if I pass the same label to both with and without, what happens?). Therefore, to avoid that case, I decided to just make them mutually exclusive. There is no subset of labels you can view using one method that you can't view using the other.

This comment has been minimized.

Member

jalabort replied Aug 14, 2014

I see... This sounds sensible. I was just wondering if it'd be possible to visualize a particular label or group of labels without having to render their actual labels (as the numbers and legend associated to them). The previous modification would allow us to do that. But you're right, it will change the way in which with_labels and without_labels work. Basically, with_labels would take precedence over without_labels...

Is this something that sounds sensible? The benefit would be to be able to easily view a particular label or groups of labels without the need of rendering their "labels" which might come handy when preparing a paper...

This comment has been minimized.

Contributor

patricksnape replied Aug 14, 2014

Just tested it and you are totally right, render_labels functionality is currently broken when it comes to selecting sub groups because it isn't propagated properly in to the landmark group (I think). Can you make an issue for this?

%matplotlib inline
# Render specific label with and without 'number labels'
# With (Works)
takeo.view_landmarks(group_label='ibug_68_points', with_labels='chin')

# Without (Broken)
# Expected: Should show only the chin label without numbering
# Actual: Shows all labels without numbering
takeo.view_landmarks(new_figure=True, group_label='ibug_68_points', with_labels='chin', render_labels=False)

This comment has been minimized.

Member

jalabort replied Aug 14, 2014

I think the previous: if render_labels: is failing to modify the variable lmark_group appropriately when render_labels==False and consequently lmark_group still contains the original group with all its labels. That's why the code ends up displaying all labels (without rendering their "labels" which is correct) instead of the selected ones...

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