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

LandmarkGroup.init_with_all_label, init_* convenience constructors #533

Merged
merged 9 commits into from Feb 11, 2015

Conversation

Projects
None yet
2 participants
@patricksnape
Contributor

patricksnape commented Jan 8, 2015

Adds a simple class method for creating landmark groups without constructing the OrderedDict yourself.

create_with_all_label

Update the labellers to use this new method.

patricksnape added some commits Jan 8, 2015

Create LandmarkGroup with all label
Adds a class method that allows simple construction of a
LandmarkGroup from a PointCloud with a single label called 'all'.
This label contains all the groups.
@jabooth

This comment has been minimized.

Member

jabooth commented Jan 14, 2015

@patricksnape looks great. One comment - I like the pattern of @classmethod convenience constructors, but maybe we should make the call signature style consistent throughout menpo.

We presently have three @classmethod constructors:

Image.blank(shape, n_channels=1, fill=0, dtype=np.float)
Rotation.from_2d_ccw_angle(theta, degrees=True)
Transform.identity(cls, n_dims)

I believe I chose these signatures, and mainly went for obviousless and readability.

We could set it as Class.create_...() - then users who might expect a class convenience constructor know to try tab completing on .crea.. to see if there are any present.

If so, maybe there is an argument for updating those signatures above to match that style? (e.g. Image.create_blank(...))

Another candidate would be Class.init_...() then we match Python's nomenclature for object creation (e.g. Rotation.init_from_2d_ccw_angle(...). Tbh I think this would be the most 'correct' and obvious pattern, and FWIW, that is how it's done in ObjC.

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jan 14, 2015

Don't quote Objective C to me as a good example of doing anything 😛
I prefer the create convention because init doesn't mean anything and init_from_2d_ccw_angle reads horribly. Presumably this would change to `create_2d_ccw_angle``?

@jabooth

This comment has been minimized.

Member

jabooth commented Jan 14, 2015

Don't quote Objective C to me as a good example of doing anything 😛

Haha well let's leave that to one side then... ;)

I prefer the create convention because init doesn't mean anything

Surely there is merit in it being __init__ not __create__ that is the only way to initialise objects in Python?

There is nothing wrong with create, but it is as equally obvious as build, construct or make to me. Guido already decided on the nomenclature for creating objects in Python and he chose init, so why would we go off the beaten track and make our own term up?

init_from_2d_ccw_angle reads horribly.

I find that reads rather well.. although I accept maybe I am used to this from iOS development. Does it not exactly describe the functionality though?

Presumably this would change to create_2d_ccw_angle?

No I don't think so. We aren't making an 'Angle', we are making a Rotation. It's just that we are building a 2D Rotation that moves things by a certain angle in a CCW sense and we only need to know the angle to build it. Rotation.from_2d_ccw_angle encapsulates all those stipulations in the most compact way I can think of.

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jan 14, 2015

Meh, this is becoming a rabbit hole discussion about the merits of naming patterns. In short, I don't mind, I just prefer non 'sentence' methods like our crop_to_landmarks_proportion_inplace. But such is the nature of not having overloads.

In short, I don't really mind, I defer the decision to you. This pull request will wait until 0.4.0 is released anyway.

@jabooth

This comment has been minimized.

Member

jabooth commented Jan 14, 2015

OK, and fair point, there's no right answer here. And I agree it can wait till 0.4.0 is out of the door.

@jabooth jabooth added this to the 0.4.1 milestone Jan 14, 2015

@patricksnape patricksnape modified the milestones: 0.4.1, 0.5.0 Jan 23, 2015

@jabooth jabooth changed the title from Landmark group create to LandmarkGroup.init_with_all_label, init_* convenience constructors Feb 11, 2015

@jabooth

This comment has been minimized.

Member

jabooth commented Feb 11, 2015

I've done the tidy up work mentioned above now - now we consistently use .init_* as the naming scheme for all @classmethod convenience constructors.

jabooth added a commit that referenced this pull request Feb 11, 2015

Merge pull request #533 from patricksnape/landmark_group_create
LandmarkGroup.init_with_all_label, init_* convenience constructors

@jabooth jabooth merged commit cf518dd into menpo:master Feb 11, 2015

2 checks passed

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

@jabooth jabooth deleted the patricksnape:landmark_group_create branch Feb 11, 2015

@jabooth jabooth removed the in progress label Feb 11, 2015

@jabooth jabooth referenced this pull request Feb 11, 2015

Merged

Init classmethods #19

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