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

BREAKING LandmarkGroups are now Shapes #675

Merged
merged 18 commits into from Feb 7, 2017

Conversation

Projects
None yet
3 participants
@patricksnape
Contributor

patricksnape commented Feb 11, 2016

This does the bulk (I think) of the work for changing
LandmarkGroup to being a shape. A lot of files are touched
because the .lms property (which has been deprecated) was being
used all over the shop.

As discussed with @jabooth and @nontas, this essentially does
the following:

  • LandmarkGroup is now a PointUndirectedGraph with special
    labelling abilities.
  • LandmarkGroup is no longer a MutableMapping and now has
    explicit methods for getting/setting labels. Ergo LandmarkGroup
    is no longer iterable.
  • LandmarkGroup label mutation maintains immutability - so
    whenever labels are added or removed a new object is returned.
  • .lms and .n_landmarks are deprecated but maintained for
    compatibility
  • A dummyLandmarkGroup is maintained in menpo.landmark for
    pickle compatibility - unpickling old-style LandmarkGroup
    classes will raise a warning and actually return a new
    style class.
  • All labels have been updated for the new class.

TODO:

  • Thoroughly check visualization - this includes reviewing
    the function of the view_landmarks method.
  • Make sure the new unpickling is actually unit tested
  • Fix all the widgets
Holy macaroni - Huge commit
This does the bulk (I think) of the work for changing
LandmarkGroup to being a shape. A lot of files are touched
because the .lms property (which has been deprecated) was being
used all over the shop.

As discussed with @jabooth and @nontas, this essentially does
the following:

  - LandmarkGroup is now a PointUndirectedGraph with special
    labelling abilities.
  - LandmarkGroup is no longer a MutableMapping and now has
    explicit methods for getting/setting labels
  - LandmarkGroup label mutation maintains immutability - so
    whenever labels are added or removed a new object is returned.
  - .lms and .n_landmarks are deprecated but maintained for
    compatibility
  - A dummy LandmarkGroup is maintained in menpo.landmark for
    pickle compatibility - unpickling old-style LandmarkGroup
    classes will raise a warning and actually return a new
    style class.
  - All labels have been updated for the new class.

TODO:
  - Thoroughly check visualization - this includes reviewing
    the function of the 'view_landmarks' method.
  - Make sure the new unpickling is actually unit tested
  - Fix all the widgets
Merge remote-tracking branch 'upstream/master' into labelled_pointgraph
Conflicts:
	menpo/landmark/base.py
Conflict was just the marker size changing down to 5 (and the docs)

@patricksnape patricksnape force-pushed the patricksnape:labelled_pointgraph branch from fc8567e to ba8ab57 Feb 18, 2016

patricksnape added some commits Feb 27, 2016

Merge remote-tracking branch 'upstream/master' into labelled_pointgraph
Conflicts:
	menpo/feature/test/test_features.py
	menpo/image/base.py
	menpo/image/boolean.py
	menpo/image/masked.py
	menpo/io/input/extensions.py
	menpo/io/input/landmark.py
	menpo/io/input/landmark_image.py
	menpo/shape/__init__.py
	menpo/shape/graph_predefined.py
Allow masking to remove all vertices from a PointGraph
This is useful because of LandmarkGroups and empty graphs
are technically allowed.
@@ -219,6 +219,8 @@ def test_export_landmark_ljson_nan_values(mock_open, exists):
# wrote null values
first_null = mock_open.mock_calls[97][1][0][1:].strip()
second_null = mock_open.mock_calls[98][1][0][1:].strip()
print(first_null)

This comment has been minimized.

@patricksnape

patricksnape Dec 1, 2016

Contributor

Yuck

This comment has been minimized.

@jabooth

jabooth Dec 1, 2016

Member

my bad sorry. That test though...pretty grim. It's the only failing one atm, you remember the logic here?

This comment has been minimized.

@patricksnape

patricksnape Dec 3, 2016

Contributor

what do you mean, how can hardcoded indices into a random string be a bad test 😛

@nontas

This comment has been minimized.

Member

nontas commented Dec 2, 2016

As I am working on updating menpowidgets to incorporate these changes, I have the following remarks:

  1. The new LandmarkGroup is Landmarkable, thus it has the .landmarks property as well as view_landmarks(). This doesn't make sense to me. I completely agree that a PointCloud or a PointGraph must be Landmarkable, but the LandmarkGroup should not.

  2. If we want the LandmarkGroup to be Landmarkable, then I think we should rename it to indicate more clearly that it belongs to the PointCloud family of menpo.shape. Thus, rename it to something like LabelledPointGraph. In that case, a PointCloud should also have its LabelledPointCloud version... If we follow such a scenario, then any Landmarkable object (e.g. Image, PointCloud etc.) can be landmarked with any out of PointCloud, PointGraph, LabelledPointGraph (and LabelledPointCloud).

Sorry to add extra complexity on this PR, but we need to come up with a very simple structure (that will also facilitate menpowidgets).

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Dec 3, 2016

  1. Is a consequence of our class structure and unavoidable. It's always been the case. 2D pointclouds can have 2D landmarks, but this doesn't matter. Just ignore that case for the visualisation. Visualisation is syntactic sugar so it doesn't really matter if it doesn't cover all use cases. It's much like how we don't recursively transform landmarks.
  2. Fine.
@nontas

This comment has been minimized.

Member

nontas commented Dec 13, 2016

Apart from test_export_landmark_ljson_nan_values() which is still failing, I think that this PR is ready.

@jabooth and I:

  • renamed the LandmarkGroup class to LabelledPointUndirectedGraph (it lives in menpo.shape.labelled)
  • fixed the docs to completely remove all references to LandmarkGroup
  • fixed import_landmark_file and export_landmark_file to successfully deal with any kind of shape (PointCloud, PointGraph, LabelledPointUndirectedGraph).

I'd like to get this in asap so that I can move on with the PRs that upgrade menpowidgets to ipywidgets5.

@nontas

This comment has been minimized.

Member

nontas commented Dec 14, 2016

We need to fix menpo3d with respect to this PR. Specifically menpo3d.io.input.landmark_mesh.py.

@jabooth

This comment has been minimized.

Member

jabooth commented Dec 14, 2016

@nontas so we should bump the version of menpo before merging this PR to v0.8.0dev0, then we are free to fix master menpo3d and get that merged.

I can look at this tomorrow.

include labels that refer to subsets of the underlying points that represent
interesting semantic *labels*. These :map:`PointCloud` or
:map:`LabelledPointUndirectedGraph` (or subclasses) are stored under
string keys - these keys are refereed to as the **group name**. A special

This comment has been minimized.

@patricksnape

patricksnape Dec 21, 2016

Contributor

referred

@patricksnape patricksnape referenced this pull request Jan 5, 2017

Open

Add tem importer #769

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jan 9, 2017

@jabooth bump to v0.8.0dev0 then get this in?

@patricksnape patricksnape changed the title from WIP: BREAKING LandmarkGroups are now Shapes to BREAKING LandmarkGroups are now Shapes Jan 9, 2017

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Feb 6, 2017

@jabooth What are we waiting on for this? We need this in before LJSONv3

@jabooth

This comment has been minimized.

Member

jabooth commented Feb 7, 2017

TODO:

  • Bump to v0.8.0dev0

Nice to do (otherwise menpo3d/menpo master broken for a while)

  • Create PR on menpo3d to fix up to this (inc bump to menpo v0.8.*)
@jabooth

This comment has been minimized.

Member

jabooth commented Feb 7, 2017

@patricksnape currently this PR causes failures in menpo3d test suite as in the new landmarks regime this is no longer possible:

mesh.landmarks['LJSON']['eye']

Now that

mesh.landmarks['LJSON']

returns any Shape, if the user knows that shape is a LabelledUndirectedGraph, this object could support __setattr__/__getattr__ for it's groups so the above would work.

To disambiguate for others if what I'm saying isn't clear:

labelled_pg = LabelledUndirectedPointGraph(...)
# we could support this for accessing groups
# then above access pattern (which legacy code
# may need) would continue to work.
print(labeled_pg['eye'])

However, I believe we discussed this and decided that:

  1. Most users didn't really understand the subtlety of what the two levels of keying meant on landmarks (i.e. img.landmarks['A_GROUP']['A_LABEL'])
  2. Labels turned out to be an underused feature to be accessed directly in such a manner, but are fantastic for visualisation.
  3. There is a more capable API for selecting labels - .with_labels(labels) and .without_labels(labels). __getattr__ would only be able to implement .with_labels(), so you would still need the other option, and now there is a confusion as there would be two wildly different ways to access labels.

The whole spirit of this PR is to simplify landmarks to try and smooth other this area of menpo (removing .lms, simplifying import/export of landmarks to simple shapes, etc). In this light, we decided not to add __getattr__ support, which means calls like the above now need to be written:

mesh.landmarks['LJSON'].with_labels('eye')

Just wanted to document this here as it's an important breaking change that may cause confusion.

@patricksnape will fix up menpo3d's tests to use with_labels() instead, and then this can come in.

jabooth added a commit to jabooth/menpo3d that referenced this pull request Feb 7, 2017

@jabooth

This comment has been minimized.

Member

jabooth commented Feb 7, 2017

OK, I'm going to go ahead and merge this in - think we are in a good spot to start polishing this in and getting it towards a release 👍

@jabooth jabooth merged commit ec01681 into menpo:master Feb 7, 2017

1 of 2 checks passed

macOS MenpoBot Jenkins build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment