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

.view_widget() on LazyList to use menpowidgets to view items #753

Merged
merged 4 commits into from May 6, 2017

Conversation

Projects
None yet
3 participants
@jabooth
Member

jabooth commented Nov 18, 2016

It's increasingly common in Menpo to use LazyList to represent homogeneous collections of menpo objects, not only because LazyList is the default return type from menpo.io's multiple import, but also because .map() on these lists to derive further lazy data is a very powerful paradigm for exploratory scientific computing.

Viewing lazy list content with widgets typically looks like this currently:

from menpowidgets import visualize_images
visualize_images(mio.import_images('./foo'))

whilst this is fine, it's a very common operation, and we can do better.

I've started exploratory work on menpowidgets to add a visualize_list convieniece function, that looks at the type of the first item in a list and invokes the most appropriate visualize_foo method on a list from this:
https://github.com/nontas/menpowidgets/blob/mm_widgets/menpowidgets/items.py
This PR, which has to wait for that change to land before it can be added, adds a simple .view() method to LazyList which simply invokes that function on the list if menpowidgets is installed. That means we can now express the above as:

mio.import_images('./foo').view()

which is much more in-line with the usual menpo paradigm for object visualization.

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Nov 18, 2016

Hmm - and heterogenous lists?

@jabooth

This comment has been minimized.

Member

jabooth commented Nov 20, 2016

@patricksnape I don't think that's really worth the complexity. It's very difficult to ensure that a lazy list is homogenous without loosing the lazy property (you'd have to execute all callables to determine their return type to really enforce the homogeneity constraint).

You could say that for index callables we assume the type of the callable return is fixed, but this is still an assumption - the callable could behave differently for different index inputs.

I know this is a little gross, but I believe here the practical connivence trumps that. Thoughts?

@jabooth

This comment has been minimized.

Member

jabooth commented Nov 20, 2016

PS I guess I final option is a HomogeneousLazyList subclass that just adds this view method for now and potentially adds other things down the line, without really having any guarantee beyond us using it in our APIs when we know the callables are homogeneous (i.e. everywhere) but again I think that's an added complexity that users don't need. It's a learning curve as it is to explain to people what a lazy list is - at least for now it's a single unified type. Don't like the idea of making that story any more complex without very good reason...

@nontas

This comment has been minimized.

Member

nontas commented Nov 23, 2016

I think we should call the method .view_widget() to be consistent with the rest of Menpo.

@jabooth

This comment has been minimized.

Member

jabooth commented Nov 23, 2016

@nontas yeah that's definitely fair. I think on another note it might be worth revisiting that pattern (kind of linked to #752), but totally agree for now it should be consistent so I'll change it.

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Nov 25, 2016

I know what you're doing @jabooth, you sneaky rascal. You're just trying to sneak in your custom list types again...

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Dec 8, 2016

Does this require anything else?

@jabooth

This comment has been minimized.

Member

jabooth commented Dec 8, 2016

@patricksnape it's just blocked on menpowidgets having the proposed visualize_list function - currently that's on a messy branch and @nontas is in the middle of fixing everything/moving it to ipywidgets 5.

@nontas nontas referenced this pull request Dec 14, 2016

Merged

Widgets Upgrade to IPywidgets6 #22

2 of 2 tasks complete

@jabooth jabooth changed the title from adds .view() on LazyList to use menpowidgets to view items to .view_widget() on LazyList to use menpowidgets to view items May 6, 2017

@jabooth

This comment has been minimized.

Member

jabooth commented May 6, 2017

unblocked this now as we have menpowidgets released with the view_widget function.

Will merge after tests pass!

@jabooth jabooth merged commit 376d223 into menpo:master May 6, 2017

2 checks passed

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

@jabooth jabooth deleted the jabooth:visualize_list branch May 6, 2017

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