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

Serialize fit results #445

Merged
merged 7 commits into from Sep 16, 2014

Conversation

Projects
None yet
2 participants
@patricksnape
Contributor

patricksnape commented Sep 15, 2014

This isn't ready to be fully pulled in yet, but it at least partially implements what we were talking about for serialisation: two basic classes that allow you to store the necessary information for fitting results.

patricksnape added some commits Sep 15, 2014

Fix some docstrings
Raise not implemented errors for cost methods
Add the concept of a serializable fitting result
It basically just contains the shapes, a copy of the parameters
and the initial image. It needs comment strings and discussing.

@jabooth jabooth added the in progress label Sep 15, 2014

self._gt_shape = gt_shape
self.parameters = None

This comment has been minimized.

@patricksnape

patricksnape Sep 15, 2014

Contributor

All fitting results have parameters now, it just defaults to None

This comment has been minimized.

@jabooth

jabooth Sep 16, 2014

Member

So I wonder if it's better if this constructor takes parameters as an argument and sets it on self:

def __init__(self, image, parameters, gt_shape=None):
    self.image = image
    self._gt_shape = gt_shape
    self.parameters = parameters

then it's clear that subclasses have to provide the parametrization on initialization. Or is this a problem with MultilevelFittingResult not having a direct parametrization?

super(NonParametricFittingResult, self).__init__(image,
gt_shape=gt_shape)
self.fitter = fitter
self._shapes = shapes

This comment has been minimized.

@patricksnape

patricksnape Sep 15, 2014

Contributor

Rename to shapes instead of parameters

@@ -242,8 +226,8 @@ def error_images(self):
:type: `list` of :map:`Image` or subclass
"""
return _flatten_out(
[f.error_images for f in self.fitting_results])
return list(chain(

This comment has been minimized.

@patricksnape

patricksnape Sep 15, 2014

Contributor

Use itertools instead of a custom method

patricksnape added some commits Sep 15, 2014

Fix various bugs in fitting images
We need the parameters to be a list within the fitting
framework. I also updated the tests to be passing.
parameters = [p.copy() for p in self.parameters]
else:
parameters = []
gt_shape = self.gt_shape.copy() if self.gt_shape else None

This comment has been minimized.

@jabooth

jabooth Sep 16, 2014

Member

how the hell have I missed that this is a valid Python construct the last 2 years?! Nice

This comment has been minimized.

@jabooth

jabooth Sep 16, 2014

Member

(have tried the old ternary if x = a < b ? y : None but never bothered to see if it was there in a more Pythonic guise)

else:
return self.parameters

This comment has been minimized.

@jabooth

jabooth Sep 16, 2014

Member

is there a good reason for this ability to choose PointCloud or not? Feels pretty redundant (I know you were just adapting what is already there but might be a good opportunity to tidy things up a little)

"""
def __init__(self, image, parameters, shapes, gt_shape):
FittingResult.__init__(self, image, gt_shape=gt_shape)
HDF5able.__init__(self)

This comment has been minimized.

@jabooth

jabooth Sep 16, 2014

Member

no need to initialize HDF5able (doesn't harm but adds noise). Maybe we should discuss this in person actually...

downscale, n_iters):
SerializableFittingResult.__init__(self, image, parameters, shapes,
gt_shape)
HDF5able.__init__(self)

This comment has been minimized.

@jabooth

jabooth Sep 16, 2014

Member

again, HDF5able is more an interface so no state to initialize

patricksnape added some commits Sep 16, 2014

A bit of a dirty commit
1. Change shapes to a property, remove the as_points option
2. Update the compute error to reflect the above change
3. Change shapes to parameters on NonParametric
4. Remove HDF5 Init
Refactor multilevel to contain multiple serialized
Now the serialized multilevel acts more like the normal multilevel
and just contains a list of serialized fitting results
self.image = image
self.fitter = fitter
self._gt_shape = gt_shape

This comment has been minimized.

@jabooth

jabooth Sep 16, 2014

Member

does this really need to be underscored?

This comment has been minimized.

@jabooth

jabooth Sep 16, 2014

Member

hmm ok I see we want setters in subclasses.

@jabooth

This comment has been minimized.

Member

jabooth commented Sep 16, 2014

this looks good, let's give it a go! :) +1

patricksnape added a commit that referenced this pull request Sep 16, 2014

@patricksnape patricksnape merged commit 87cbb50 into menpo:master Sep 16, 2014

1 check passed

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

@jabooth jabooth removed the in progress label Sep 16, 2014

@patricksnape patricksnape deleted the patricksnape:serialise_fit_results branch Sep 16, 2014

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