-
Notifications
You must be signed in to change notification settings - Fork 13
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
wk-libs: Add Dataset.add_layer_from_images() using pims #741
Conversation
@philippotto I guess this is ready for review now. Some minor parts are still missing, see TODO, but test + code is ready. Let me know if we should do a walkthrough together. I'd also be very happy if you have a good idea how to simplify the logic in |
PS: The new tests add ~5 minutes. Probably we should look into parallelizing tests or reducing the time somehow. The downloaded datasets might also be cached in some folder (locally gitignored in the repo and via extra caching steps in the CI). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff! I already reviewed most of the PR and left some feedback, but maybe we can have a call about the pims_images.py
module, as I think I could use some kind of introduction before reviewing it :)
# if a slice is larger than the others it might happen that | ||
# a new chunk is partially written, leading to a warning, | ||
# which is ignored in this context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this even happen if all slices have the same size but that size is not shard-aligned? also, why is the warning hidden? the user could set compress=False to avoid the performance penalty, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the first case this should work, since the first write will set the new bounding-box, which then fits all subsequent writes. Writes must be either shard or bounding-box aligned (per bbox-border). I think this warning is not useful at all in this case, since it only happens at the dataset-borders, where the user can not change much. The only important warning is about the different sizes, which is handled elsewhere. The performance penalty should be negligible, since it's only about the borders of the dataset. It's not that the general chunk-size doesn't fit the blocks during iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, please explain this in the code comment too then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expanded the comment, I hope it makes more sense now 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome stuff! didn't look at the tests, yet, but I already have some feedback :)
@philippotto Thanks a lot for the review! I think I adressed all points, either in the newest commits or commenting there directly. Please check my newest changes again, thanks 🙏 |
ping @philippotto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review! I only left some smaller comments (mostly about code comments) :)
# if a slice is larger than the others it might happen that | ||
# a new chunk is partially written, leading to a warning, | ||
# which is ignored in this context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, please explain this in the code comment too then :)
Co-authored-by: Philipp Otto <[email protected]>
Co-authored-by: Philipp Otto <[email protected]>
Description:
Adds
Dataset.add_layer_from_images()
, which converts plenty of image (stack) formats to a wk-compatible layer.For the usage, please check the added test. This can also be executed standalone, which will upload the resulting datasets to the webknossos instance configured in your
.env
, I'd recommend to use a local wk instance. This helps to inspect the outputs manually.I moved the respective testfiles to the webknossos package and linked them from wkcuber, since changes in webknossos also trigger CI runs for wkcuber, but not the other way around.
I also adapted the warning behaviour for multiprocessing the in the cluster-tools. Should we also adapt this for the other executors? Might be out-of-scope for this PR though.
Issues:
Todos: