Skip to content

Add support for multiple volume layers in annotations #588

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

Merged
merged 17 commits into from
Feb 11, 2022

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Feb 8, 2022

Description:

  • If ambiguous, the layer name has to be provided when reading from a volume layer in an annotation.

This PR is only fully compatible with webKnossos once scalableminds/webknossos#6028 (comment) is merged, since that PR fixes a naming issue in annotation downloads.

Issues:

Todos:

Make sure to delete unnecessary points or to check all before merging:

  • Updated Changelog

@philippotto philippotto self-assigned this Feb 8, 2022
@philippotto philippotto requested a review from jstriebel February 8, 2022 15:21
Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

Nice! Mostly LGTM, I left a few minor comments, but approving already 👍

Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

🎉

@philippotto
Copy link
Member Author

@jstriebel mypy had some troubles with the _get_name_or_id method which is why I had to inline it 🤷 by the way, I noticed that mypy cannot really do typechecking on properties which use @cachedproperty. I noticed this because after I removed _get_name_or_id, mypy didn't complain even though it was still used. The decorator is currently only used in the annotation class. I briefly tried a newer mypy version because I saw python/typing#985, but this didn't fix the issue. Just FYI..

@jstriebel
Copy link
Contributor

@jstriebel mypy had some troubles with the _get_name_or_id method which is why I had to inline it 🤷

Weird error, but glad you found an easy fix 👍

by the way, I noticed that mypy cannot really do typechecking on properties which use @cachedproperty. I noticed this because after I removed _get_name_or_id, mypy didn't complain even though it was still used. The decorator is currently only used in the annotation class. I briefly tried a newer mypy version because I saw python/typing#985, but this didn't fix the issue. Just FYI..

Good point! I guess the main problem is, that boltons (from where we importcachedproperty) is not typed. There are some stubs available here:
https://github.com/wcooley/python-boltons-stubs/blob/master/boltons-stubs/cacheutils.pyi#L127-L130
Seems like this is not published to pypi, but we can add them (or at least the ones we need) to our repo: https://mypy.readthedocs.io/en/stable/stubs.html

@philippotto
Copy link
Member Author

Good point! I guess the main problem is, that boltons (from where we importcachedproperty) is not typed. There are some stubs available here:
https://github.com/wcooley/python-boltons-stubs/blob/master/boltons-stubs/cacheutils.pyi#L127-L130
Seems like this is not published to pypi, but we can add them (or at least the ones we need) to our repo: https://mypy.readthedocs.io/en/stable/stubs.html

Ah, makes sense 👍 I added the cachedproperty stubs, since I consider them relatively important for the type coverage of the annotation module.

@@ -20,7 +20,7 @@ def main() -> None:

# Step 1: Download the dataset and our training data annotation from webKnossos to our local computer
training_data_bbox = wk.BoundingBox.from_tuple6(
annotation.skeleton.user_bounding_boxes[0]
annotation.skeleton.user_bounding_boxes[0] # type: ignore[index]
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pushing this. Yesterday, I was unsure how to fix this (strictly speaking one would need to check that user_bounding_boxes is not None). Ideally, it would always default to an empty list. However, that's likely out-of-scope for this PR. So, let's tackle it at a different point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, cleaning things up a bit more in the skeleton class is definitely a good idea, but let's defer this.

Copy link
Contributor

Choose a reason for hiding this comment

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

(and the assertion would clutter the user-facing example to much in my opinion, so i went for the ignore)

@philippotto philippotto merged commit 1cb2aa1 into master Feb 11, 2022
@philippotto philippotto deleted the multi-volume-support branch February 11, 2022 09:18
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.

Adapt volume annotation to support multiple layers
2 participants