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

Video importer docs and negative max_images #684

Merged
merged 3 commits into from Apr 11, 2016

Conversation

Projects
None yet
3 participants
@grigorisg9gr
Member

grigorisg9gr commented Apr 9, 2016

That is only ffmpeg is currently supported, avconv raises an error.

Additionally, minor fix in the multiple importers, since the max_{}
can get negative int values (does not return the last abs(max_{}) elements).

Change documentation for video importer to match the code.
That is only ffmpeg is currently supported, avconv raises an error.

Additionally, minor fix in the multiple importers, since the max_{}
can get negative int values (does not return the last abs(max_{}) elements).

@jabooth jabooth added the in progress label Apr 9, 2016

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Apr 9, 2016

I'm not sure I understand your comment about the negative numbers?

@grigorisg9gr

This comment has been minimized.

Member

grigorisg9gr commented Apr 10, 2016

If you try to give max_images = -5 in import_images, it will import all but the last 5 or so, so it still works with negative int's.

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Apr 10, 2016

Hmm well I'm not sure it's really designed to work that way. Perhaps that would be considered a bug...

@grigorisg9gr

This comment has been minimized.

Member

grigorisg9gr commented Apr 10, 2016

That's a different decision whether you would like to have it or not.

However, currently in the code the command filepaths = filepaths[:max_assets] in _import_glob_lazy_list() works with both positive and negative.

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Apr 10, 2016

Yes that is an unintended consequence of the Python slice syntax. Given the name of the argument and what it was intended to do in the first place this is a bug. For example, max_* implies you will load a maximum number of assets. If you have 100 items, and you pass -2, you will receive 98 items, not the last 2 items. So i'd rather check for this and throw a ValueError. Would be good if you could fix that here instead and then we can pull this in!

@patricksnape patricksnape changed the title from Change documentation for video importer to match the code. to Video importer docs and negative max_images Apr 11, 2016

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Apr 11, 2016

Thanks @grigorisg9gr!

@patricksnape patricksnape merged commit 4919fed into menpo:master Apr 11, 2016

4 checks passed

OS X MenpoBot Jenkins build passed
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 grigorisg9gr:minor_documentation_change_io branch Apr 11, 2016

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