Feature/streamlined annotations #327
Conversation
src/resdk/query.py
Outdated
| @@ -92,6 +98,10 @@ def __init__(self, resolwe, resource, slug_field="slug"): | |||
|
|
|||
| self.logger = logging.getLogger(__name__) | |||
|
|
|||
| def _non_string_iterable(self, item) -> bool: | |||
| """Return thur when item is iterable but not string.""" | |||
There was a problem hiding this comment.
Spelling: "thur" should be changed to True, i guess?
Also, could you explain a bit what this function is necessary?
There was a problem hiding this comment.
Before there were checks where it was expected for resources to be of instance list (_dehydrate_resources, _add_filter). I believe I used them with iterable (dictkeys...) or something like that so I changed it to a more general type.
src/resdk/query.py
Outdated
| # Execute the query in a single request. | ||
| super()._fetch() | ||
|
|
||
| missing = { |
There was a problem hiding this comment.
Cold you explain why this scenario happens? Why in some cases self._field is None, while in others it is not None?
There was a problem hiding this comment.
This is only optimization: the annotation fields are fetched in a single query when reading corresponding annotation values.
Maybe this is not necessary. Consider the following use case:
values = res.annotation_values.filter(entity=xxx)
for value in values:
print(value.field.name, value.field.group.name)
This would query the backend for every field (so it would produce n queries). The "optimized" fetch only produces two queries (one for values and one for fields). One could argue that fields are not always used so the second query is not always necessary.
src/resdk/resources/annotations.py
Outdated
|
|
||
| READ_ONLY_FIELDS = BaseResource.READ_ONLY_FIELDS + ("label",) | ||
|
|
||
| UPDATE_PROTECTED_FIELDS = BaseResource.UPDATE_PROTECTED_FIELDS + ("entity", "field") |
There was a problem hiding this comment.
The filed entity is called sample in resdk. Could you enable that users would be able to relate to the sample as AnnotationValue.sample? However, this will only return Sample ID. So maybe it is better to expose this as AnnotationValue.sample_id... We can discuss this live.
gregorjerse
force-pushed
the
feature/streamlined_annotations
branch
2 times, most recently
from
cf8b1dd to
8b889ec
Compare
src/resdk/query.py
Outdated
| :raises LookupError: when field at the specified path does not exist. | ||
| """ | ||
| group_name, field_name = full_path.split(".", maxsplit=1) | ||
| return self.get(field__name=field_name, field__group__name=group_name) |
There was a problem hiding this comment.
This will probably return all AnnotationValues with given field. So it is probably self.filter(...) instead of self.get(...)
There was a problem hiding this comment.
I see, the backend refuses to provide result unless one entity filter is provided. However, my comment is still relevant, consider this filter:
res.annotation_value.filter(entity__collection=<collection>).from_path("general.species")
This will result in many results and AnnotationValueQuery.get fails since it got multiple results.
Please add a comment in docstring that indicates that user needs to provide a sample (entity) filter.
gregorjerse
force-pushed
the
feature/streamlined_annotations
branch
9 times, most recently
from
e397f55 to
caa1269
Compare
gregorjerse
force-pushed
the
feature/streamlined_annotations
branch
from
caa1269 to
0452f17
Compare
| """Get annotation group.""" | ||
| return self._group | ||
|
|
||
| @group.setter |
There was a problem hiding this comment.
group is among READ_ONLY_FIELDS , while it also has a setter method. Isn't this a bit strange? I imagine that group should not be changed one the field is created...
src/resdk/resources/annotations.py
Outdated
| READ_ONLY_FIELDS = BaseResource.READ_ONLY_FIELDS + ( | ||
| "field", | ||
| "entity", | ||
| "value", |
There was a problem hiding this comment.
Fields field and value are among other fields already.... Also, why is entity here?
src/resdk/resources/annotations.py
Outdated
| return self._field | ||
|
|
||
| @field.setter | ||
| def field(self, payload): |
There was a problem hiding this comment.
Consider raising error in case _field is not None. This would prevent users from believing they updated the field immediately. I realize that this would raise error during save()
src/resdk/query.py
Outdated
| :raises LookupError: when field at the specified path does not exist. | ||
| """ | ||
| group_name, field_name = full_path.split(".", maxsplit=1) | ||
| return self.get(field__name=field_name, field__group__name=group_name) |
There was a problem hiding this comment.
I see, the backend refuses to provide result unless one entity filter is provided. However, my comment is still relevant, consider this filter:
res.annotation_value.filter(entity__collection=<collection>).from_path("general.species")
This will result in many results and AnnotationValueQuery.get fails since it got multiple results.
Please add a comment in docstring that indicates that user needs to provide a sample (entity) filter.
gregorjerse
force-pushed
the
feature/streamlined_annotations
branch
from
ecce189 to
eac0922
Compare
gregorjerse
force-pushed
the
feature/streamlined_annotations
branch
from
eac0922 to
f56898a
Compare
No description provided.