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

expensive properties should really be methods #470

Merged
merged 4 commits into from Oct 12, 2014

Conversation

Projects
None yet
1 participant
@jabooth
Member

jabooth commented Oct 9, 2014

Computed properties are great for keeping code clean. Small computations or remapping's of values (e.g. pixels.shape[0] == width) are excellent candidates for properties, because the computation is fast. This is important, as in general the user doesn't know that any computation is going on - when coding, using a property feels the same as accessing an attribute (cheap and fast) regardless of how expensive the call actually is. For this reason, expensive properties can be confusing.

As an example, say we have the following:

n_x = mesh.vertex_normals[0]
n_y = mesh.vertex_normals[1]
n_z = mesh.vertex_normals[2]

This doesn't look like a particularly troubling block of code, but in reality vertex_normal is an expensive computed property, so it would be appropriate (and 3x faster) to do the following:

vn = mesh.vertex_normals
n_x = vn[0]
n_y = vn[1]
n_z = vn[2]

The issue is that the user has no way of knowing that this is an expensive operation that should be cached in such a manner. There is a simple change that makes the behavior much more apparent - just make vertex_normals a method!

vn = mesh.vertex_normals()
n_x = vn[0]
n_y = vn[1]
n_z = vn[2]

Now the user would think twice about calling the method needlessly many times.

This PR goes through the shape module and changes a number of properties to methods. The rule I've tried to follow is -

would I wince if I saw the property being called multiple times in a row?

The ones I've done here are:

  • PointCloud.h_points
  • PointCloud.centre
  • PointCloud.centre_of_bounds
  • Mesh.vertex_normals
  • Mesh.face_normals
  • AlignmentTransform.aligned_source
  • AlignmentTransform.alignment_error
  • GeneralizedProcrustesAnalysis.mean_alignment_error (was av_alignment_error)
  • GeneralizedProcrustesAnalysis.mean_aligned_shape
  • InvertableTransform.pseudoinverse

@jabooth jabooth added the in progress label Oct 9, 2014

jabooth added a commit that referenced this pull request Oct 12, 2014

Merge pull request #470 from jabooth/methodsproperties
expensive properties should really be methods

@jabooth jabooth merged commit 82f58e7 into menpo:master Oct 12, 2014

2 checks passed

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

@jabooth jabooth removed the in progress label Oct 12, 2014

@jabooth jabooth deleted the jabooth:methodsproperties branch Oct 12, 2014

@jabooth jabooth referenced this pull request Oct 14, 2014

Closed

BoundingBox #477

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