Skip to content
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

Improve flexibility of IO #10

Merged
merged 13 commits into from
Nov 13, 2019
Merged

Conversation

clbarnes
Copy link
Contributor

  • Adds zarr and pyn5 (for cases where conda isn't available)
  • Makes group/dataset checking more flexible by expanding register_filetype
  • Reduces duplicated code in io_tests using inheritance of test classes
  • Explicitly adds optional dependencies to travis environment, as they weren't being picked up before

@clbarnes clbarnes mentioned this pull request Nov 13, 2019
@constantinpape
Copy link
Owner

Looks pretty cool, I will have a closer look at it later.

@clbarnes
Copy link
Contributor Author

clbarnes commented Nov 13, 2019

A bit tricky to abstract over zarr's stores given they don't have __enter__ and __exit__ methods for context manager, I've used a zarr_open function which wraps zarr.open and patches them in but I'm not happy about it.

Copy link
Owner

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good overall. If we can agree on a better way to structure the dependencies, we could already integrate this in this PR.

.travis.yml Show resolved Hide resolved
elf/io/extensions.py Show resolved Hide resolved
elf/io/extensions.py Show resolved Hide resolved
elf/io/files.py Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Copy link
Owner

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clbarnes LGTM.
Is there anything else you want to add to this PR?

@clbarnes
Copy link
Contributor Author

I think that's all for now! Thanks.

@constantinpape
Copy link
Owner

Thanks for the contribution!

@constantinpape constantinpape merged commit 1313540 into constantinpape:master Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants