Annotation usage simplification #80
Conversation
-- Collection -- - Grammar/docstring/format updates. - Proper caching for lookups (v1 and v2). - Utility functions in Collection to hide private methods (particularly around using lookups to swap from image/row spaces). - Enhanced checking and exception raising. - Multi-threaded save/download actions now check for exceptions in worker threads. -- Annotation -- - Grammar/docstring/format updates. - Moved reliance on internal data to getters to reduce chance of errors. - Removed unnecessary constructor in AnnotationMask. - Moved boolmask checking into dedicated static method, with a choice to parse as [h, w, N] or [h, w] arrays.
ahaith
left a comment
ahaith
left a comment
There was a problem hiding this comment.
Looks fine although in future better to do these kinds of changes as a set of small self-contained PRs, even if they're just little ad-hoc fixups.
There does seem to be a missing function here so please look into that - would be easier to notice that if this PR was just changes to the annotation stuff!
zegami_sdk/annotation.py
Outdated
| """ | ||
| !! STOP !! Instantiate a non-hidden subclass instead. | ||
| def __init__(self, collection, id, row_index, source=0): | ||
| """!! STOP !! Instantiate a non-hidden subclass instead. |
There was a problem hiding this comment.
Check out AbstractBaseClass which makes it impossible to instantiate the base class. https://docs.python.org/3/library/abc.html
There was a problem hiding this comment.
Solved in new commit, doesn't actually require abc as this functionality is enforced a different way
zegami_sdk/annotation.py
Outdated
| self._imageset_id = collection._get_imageset_id(source=source) | ||
| self._row_index = row_index | ||
|
|
||
| self._data = collection.fetch_annotation_data(self._row_idx) # { imageset_id, image_index, type, annotation } |
There was a problem hiding this comment.
As things stand, annotations apply to the specific image (i.e. source) So the same row could have an annotation for the image of one source, but not for the other. The same annotation could even be associated to multiple rows if multiple rows refer to the same image.
There are clearly scenarios where it'd make more sense for the annotation to transcend the sources, but also cases where it doesn't.
Not sure how best to deal with that, perhaps include source in fetch_annotation_data?
There was a problem hiding this comment.
In fact, I don't see fetch_annotation_data in collection.py. Is there a change missing from this PR?
There was a problem hiding this comment.
Not sure how this creeped in, my version doesn't even use this any more! New commit fixes
zegami_sdk/collection.py
Outdated
| def download_annotation(self, annotation_id): | ||
| """ | ||
| Converts an annotation_id into downloaded annotation data. | ||
| """Converts an annotation_id into downloaded annotation data. |
There was a problem hiding this comment.
Multiline docstrings in our other python repos have the opening """ on its own line (as per before this change).
I don't have a strong opinion about this, but it does seem better to have a consistent style.
There was a problem hiding this comment.
All of collection.py and annotation.py fixed up to suit this
Also fixed all over-line-limits on collection.py
-- Collection --
-- Annotation --