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

BREAKING: Add LazyList - default importing is now Lazy #669

Merged
merged 6 commits into from Feb 8, 2016

Conversation

Projects
None yet
2 participants
@patricksnape
Contributor

patricksnape commented Feb 5, 2016

This adds the concept of an Immutable LazyList class. This
is essentially a very simple class whereby the elements are
callables that are assumed to do something on index. This is
very useful for importing because we just immediately return
a set of partial import methods that wrap the file paths that
need to be imported. The disadvantage is obviously that if the
file system changes after the list is constructed then the
import will fail. However, the low memory and instant access
(think random access) is a great benefit. To allow for backwards
compatibility there is a new as_generator argument that restores
the previous behaviour.

Finally, there is one major change - now, importers that return
lists of assets will not flatten the overall list - for example,
if you import a GIF, previously the frames would be yielded as
seperate images. Instead, the LazyList will not contain a list
that contains all the frames of the GIF. If this is a serious game
changer then we can add a helper generator that will flatten
the lists. Something that neatly wraps
list(itertools.chain.from_iterable(assets)).

@jabooth This one is for you mate!

Breaking:

  • Importers now return LazyList
  • Landmark resolvers now receive paths rather than objects

patricksnape added some commits Feb 5, 2016

Add LazyList - default importing is now Lazy
This adds the concept of an Immutable LazyList class. This
is essentially a very simple class whereby the elements are
callables that are assumed to do something *on index*. This is
very useful for importing because we just immediately return
a set of partial import methods that wrap the file paths that
need to be imported. The disadvantage is obviously that if the
file system changes after the list is constructed then the
import will fail. However, the low memory and instant access
(think random access) is a great benefit. To allow for backwards
compatibility there is a new as_generator argument that restores
the previous behaviour.

Finally, there is one major change - now, importers that return
lists of assets will not flatten the overall list - for example,
if you import a GIF, previously the frames would be yielded as
seperate images. Instead, the LazyList will not contain a list
that contains all the frames of the GIF. If this is a serious game
changer then we can add a helper generator that will flatten
the lists.
@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Feb 5, 2016

Travis is failing because the current scipy release is broken 😨

Sneak in UINT16 support
We now support uint16 image types. Also, inline the PIL
import.

@patricksnape patricksnape referenced this pull request Feb 6, 2016

Merged

BREAKING: Imageio #633

A LazyList where each element returns the underlying indexable
object wrapped by ``f``.
"""
return LazyList([partial(f, i) for i in range(n_elements)])

This comment has been minimized.

@jabooth

jabooth Feb 6, 2016

Member

is the motivation for this video sequences? Can see the use case if some API vends lists of callables that take an index arg that could be dropped straight in here, just wondering.

This comment has been minimized.

@patricksnape

patricksnape Feb 6, 2016

Contributor

Yes the video API will use this method

lazy : `LazyList`
A new LazyList where each element is wrapped by ``f``.
"""
return LazyList([partial(f, x) for x in self])

This comment has been minimized.

@jabooth
the same stem as the asset.
"""
# pattern finding all landmarks with the same stem
pattern = asset.path.with_suffix('.*')

This comment has been minimized.

@jabooth

jabooth Feb 6, 2016

Member

Don't mind this change, but it is another breaking one, can you note it in the PR notes so we catch it in the changelog?

This comment has been minimized.

@patricksnape

patricksnape Feb 6, 2016

Contributor

done

n_items=n_files)
elif verbose:

This comment has been minimized.

@jabooth

jabooth Feb 6, 2016

Member

nice. :)

This comment has been minimized.

@patricksnape

patricksnape Feb 6, 2016

Contributor

Happy with that message?

@jabooth

This comment has been minimized.

Member

jabooth commented Feb 6, 2016

@patricksnape reviewed, just haven't tried it yet. Only few minor points but looks great. I'll try it later tonight. Nice work!

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Feb 7, 2016

@jabooth Any thoughts on adding a cached lazy list or not?

@patricksnape patricksnape changed the title from Add LazyList - default importing is now Lazy to BREAKING: Add LazyList - default importing is now Lazy Feb 7, 2016

@jabooth

This comment has been minimized.

Member

jabooth commented Feb 7, 2016

Hmm an as_cached() method on LazyList would be sweet:

menpo.io.import_images(...).as_cached()

Implement just as memoise-wrapping self._callables right?

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Feb 7, 2016

I was thinking like a kwarg on construction? So if you use choose to cache then it will memoise the accesses like you said. Then the overhead is free.

patricksnape added some commits Feb 8, 2016

Add tests for LazyList
Better support subclasses by using cls and __class__
Map was incorrect
Was iterating through the list rather than the underlying
callables and so was invoking them. Added a better test
to catch this.
@jabooth

This comment has been minimized.

Member

jabooth commented Feb 8, 2016

After discussions with @patricksnape this morning, we are going to hold off on implementing caching for now, see #671 for discussion.

@jabooth

This comment has been minimized.

Member

jabooth commented Feb 8, 2016

Happy for this to come in when tests pass! +1

patricksnape added a commit that referenced this pull request Feb 8, 2016

Merge pull request #669 from patricksnape/lazy_list
BREAKING: Add LazyList - default importing is now Lazy

@patricksnape patricksnape merged commit 55dc933 into menpo:master Feb 8, 2016

4 checks passed

OS X MenpoBot Jenkins build passed No test results found.
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@patricksnape patricksnape deleted the patricksnape:lazy_list branch Feb 8, 2016

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