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

Update mean_pointcloud #595

Merged
merged 3 commits into from Jun 23, 2015

Conversation

Projects
None yet
3 participants
@jalabort
Member

jalabort commented Jun 22, 2015

  • Works as expected for pointcloud subclasses i.e. an instance of these subclasses (instead of a pointcloud instance) is returned.
Update mean_pointcloud
-Works as expected for pointcloud subclasses
def mean_pointcloud(pointclouds):
r"""
Compute the mean of a `list` of :map:`PointCloud` objects.
Compute the mean of a `list` of :map:`PointCloud` or subclass objects.

This comment has been minimized.

@patricksnape

patricksnape Jun 22, 2015

Contributor

may want to mention that the list is assumed to be homogenous (since we just blindly take the first element of the list and I guess assume all items in the list are of the same PointCloud subclass)

@jabooth

This comment has been minimized.

Member

jabooth commented Jun 22, 2015

nice, but I would suggest a slight change to more correctly follow the Vectorizing machinery:

# make a temporary PointCloud (with copy=False for low overhead) 
tmp_pc = PointCloud(sum(pc.points for pc in pointclouds) / len(pointclouds), copy=False)
# use one of the provided types to rebuild from the vector
return pointclouds[0].from_vector(tmp_pc.as_vector())

This way we don't have any assumptions about how types vectorize (before we kind of relied on a reshape of a numpy array being the same as a vectorization of a PointCloud - I can't imagine we will ever change that but it's better to not rely on it).

@jalabort

This comment has been minimized.

Member

jalabort commented Jun 23, 2015

This should be good to go ;-)

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jun 23, 2015

Added some tests, once they pass +1

@jalabort

This comment has been minimized.

Member

jalabort commented Jun 23, 2015

Everyone happy with this? @jabooth @nontas ?

@jabooth

This comment has been minimized.

Member

jabooth commented Jun 23, 2015

Great, +1

jabooth added a commit that referenced this pull request Jun 23, 2015

@jabooth jabooth merged commit d4e9e2c into menpo:master Jun 23, 2015

3 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jabooth jabooth deleted the jalabort:update_mean_pc branch Jun 23, 2015

@jabooth jabooth removed the in progress label Jun 23, 2015

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