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

Exporters #407

Merged
merged 15 commits into from Jul 29, 2014

Conversation

Projects
None yet
2 participants
@patricksnape
Contributor

patricksnape commented Jul 28, 2014

Adds landmark and image exporters.

  • Uses PIL for images so supports nearly all common types.
  • Supports ljson and pts landmark formats (for now).

Fixes a few import mistakes I made.

No mesh support as of yet.

patricksnape added some commits Jul 28, 2014

Refactor __init__
Expose input methods from input folder
Fix relative imports
I messed up the style, reverting them here
Add the landmark and image exporters
Fairly simple, just follows the import style. Supports file
handles as well as strings.

Currently we can export all PIL types and LJSON and PTS style
landmarks.
Expose the exporters
Also, add lots of tests to make sure that I've been doing sensible
things!
@@ -572,7 +572,7 @@ def ibug_open_eye_points(landmark_group):
:class:`menpo.landmark.exceptions.LabellingError`
If the given landmark group contains less than 38 points
"""
from ..shape import PointGraph
from mnepo.shape import PointGraph

This comment has been minimized.

@jabooth

jabooth Jul 28, 2014

Member

mnepo, strong

Uses all the tricks in the book to expand a path out to an absolute one.
"""
return os.path.abspath(os.path.normpath(
os.path.expandvars(os.path.expanduser(filepath))))

This comment has been minimized.

@jabooth

jabooth Jul 28, 2014

Member

newline on end please

with open('/tmp/test.jpg') as f:
type(f).name = PropertyMock(return_value='/tmp/test.jpg')
mio.export_image(test_img, f, export_extension='jpg')
PILImage.save.assert_called_once()

This comment has been minimized.

@jabooth

jabooth Jul 28, 2014

Member

same here

pts = pts[:, [1, 0]] + 1

header = r"""version: 1
n_points: {}

This comment has been minimized.

@jabooth

jabooth Jul 28, 2014

Member

why break this over 3 lines?

This comment has been minimized.

@patricksnape

patricksnape Jul 28, 2014

Contributor

break what?

@@ -0,0 +1,3 @@
def PILExporter(image, file_handle):
pil_image = image.as_PILImage()
pil_image.save(file_handle)

This comment has been minimized.

@jabooth

jabooth Jul 28, 2014

Member

again newline at end

Whitespace fixes
@jabooth was offended by header string for PTS (which was split
to make it clear what it looked like) so I changed them for new
lines
from ..utils import _norm_path


def export_landmark_file(landmark_group, filepath, export_extension=None,

This comment has been minimized.

@jabooth

jabooth Jul 28, 2014

Member

sometimes we have export_extension, sometimes file_extension in other methods in this package. Could we normalize them? I think extension is very clear and concise..

@jabooth

This comment has been minimized.

Member

jabooth commented Jul 28, 2014

Two thoughts:

  1. The argument is filepath throughout, but it can be a file handle. Can we go for a name that makes it clear it can be either? numpy use file but that collides with a builtin. I've also seen fp used - suggests both FilePath and FilePointer?
  2. May be controversial, but could we consider going with the fp as first argument for all methods? Might seem less natural, but for my money it opens up for a more functional approach. For instance, we might imagine a future where we want to be able to serialise out state periodically in the middle of some expensive_process() (the state of which can be visualised as an image). It could naturally take a function to handle that as a kwarg (let's say intermediate_save). With file path as the first, I could do something like:
writer = partial(export_image, './save it here please.jpg', overwrite=True)
expensive_process(loads_of_images, intermediate_save=writer)

with filepath the other way round I have to flip args:

writer = lambda img: export_image(img, './save it here please.jpg', overwrite=True)

that's contrived of course and not that severe, but see this talk for a great example of how you can have really nice patterns if you think about argument ordering carefully.

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jul 28, 2014

fp is fine. The documentation explains it anyways, but sure.

np.savetxt has output path first, which seems reasonable. I can't imagine ever using a partial like that, because I'd be more likely to want to keep all checkpoints at different file saves, but it is consistent with with other packages like Numpy.

patricksnape added some commits Jul 29, 2014

Swap fp and object
Rename filepath to fp, as per @jabooth request
@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jul 29, 2014

@jabooth Changes made and documentation updated.

@jabooth

This comment has been minimized.

Member

jabooth commented Jul 29, 2014

Looks great. Once travis is happy we can get this in. +1.

jabooth added a commit that referenced this pull request Jul 29, 2014

@jabooth jabooth merged commit e31c1ed into menpo:master Jul 29, 2014

1 check passed

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

@jabooth jabooth deleted the patricksnape:exporters branch Jul 29, 2014

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