-
Notifications
You must be signed in to change notification settings - Fork 15
No annotations backend #390
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
base: main
Are you sure you want to change the base?
Conversation
|
/review |
|
|
||
| /** Total number of unique labels in the dataset */ | ||
| totalLabels: 71, | ||
| totalLabels: 72, |
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.
Had to increase since we now also have "No Annotations"
|
|
||
| # Define the path to the dataset directory | ||
| dataset_path = env.path("EXAMPLES_DATASET_PATH", "/path/to/your/dataset") | ||
| dataset_path = env.path("DATASET_PATH", "../../../../yolo-data-example/dataset.yaml") |
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.
Please undo changes to this file. Instead, you should use the .env local file.
| """Returns the number of samples in the collection without annotations.""" | ||
| total_no_annotations_query = ( | ||
| select(func.count()) | ||
| .select_from(ImageTable) |
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.
This select should be from SampleTable.
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.
But wait, that is strange. We use images everywhere in this file, and the filters have width and height filters. Is count_annotations_by_collection function specific for Images?
|
|
||
| def _resolve_annotation_label_ids( | ||
| session: Session, annotation_label_names: list[str] | None | ||
| ) -> list[UUID] | None: |
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.
Why do we allow None? Isn't empty list enough?
| annotation_label_ids: list[UUID] | None = None | ||
| include_unannotated_samples: bool | None = None |
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.
Please add a docstring that explains what is the meaning and how are the conditions combined.
| include_unannotated_samples: bool | None, | ||
| preserve_empty_label_ids: bool = False, | ||
| ) -> AnnotationFilter | None: | ||
| """Build an AnnotationFilter from raw filter values.""" |
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.
What is the meaning of preserve_empty_label_ids? Please add a docstring describing the args.
| def apply_to_samples(self, query: QueryType, sample_id_column: Any) -> QueryType: | ||
| """Apply annotation filters using the provided sample ID column.""" | ||
| if self.annotation_label_ids is None: | ||
| return query |
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.
So include_unannotated_samples=False is ignored if annotation_label_ids is None?
| from lightly_studio.type_definitions import QueryType | ||
|
|
||
|
|
||
| class AnnotationFilter(BaseModel): |
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.
Please move it to a separate file.
| """ | ||
| # TODO(Igor, 01/2026): Use _CountFilters as the input argument to simplify this API. | ||
| total_counts = _get_total_counts(session=session, collection_id=collection_id) | ||
| filtered_label_ids = _resolve_annotation_label_ids( |
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 it was a wrong suggestion to store the ids, names are more natural since the request comes from the user. Can you revert it? Or perhaps more practically, add a todo and do it in a follow-up?
| select(AnnotationBaseTable.parent_sample_id).select_from(AnnotationBaseTable).distinct() | ||
| ) | ||
|
|
||
| if self.annotation_label_ids is not None: |
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.
This is always True - we exit early above for the opposite condition. Let's remove the if.
| .distinct() | ||
| ) | ||
|
|
||
| if self.annotation_label_ids is not None: |
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.
Same comment as above - always True.
What has changed and why?
How has it been tested?
Did you update CHANGELOG.md?