ED-436 - adding Skeletoncoordinates parser #822
Conversation
encord/objects/coordinates.py
Outdated
| else: | ||
| raise LabelRowError(f"Invalid format for skeleton coordinates: {skeleton_dict}") | ||
| for value in sorted_dict_values: | ||
| if value.get("invisible") and value["invisible"]: |
There was a problem hiding this comment.
Checking the condition multiple times?
Likewise with the other checks.
Should be sufficient. Just to do value.get('invisible')
arthur-encord
force-pushed
the
av/create_skeletoncoordinates_instance
branch
from
72d989a to
740b5f1
Compare
Unit test report (Pydantic 1.x)195 tests 195 ✅ 5s ⏱️ Results for commit 008e6b4. ♻️ This comment has been updated with latest results. |
Unit test report ((Pydantic 2.x)195 tests 195 ✅ 6s ⏱️ Results for commit 008e6b4. ♻️ This comment has been updated with latest results. |
arthur-encord
force-pushed
the
av/create_skeletoncoordinates_instance
branch
2 times, most recently
from
1143905 to
5607bee
Compare
encord/objects/coordinates.py
Outdated
| @@ -298,7 +284,9 @@ class SkeletonCoordinate(BaseDTO): | |||
| color (Optional[str]): The color associated with the skeleton point. | |||
| feature_hash (Optional[str]): A unique hash for the feature. | |||
| value (Optional[str]): An optional value associated with the skeleton point. | |||
| visibility (Optional[Visibility]): The visibility state of the skeleton point. | |||
| visible (Optional[bool]): `True` if the skeleton point is visible within the frame (default). | |||
There was a problem hiding this comment.
This is transitioning from basically directly de-serializing the underlying JSON to a Python object and not trying to have it as an Enum which somewhat represents the domain better.
There's arguments for both: This version: Matches underlying data better. Prev version: Tries to hide some complexity from the consumer.
I think this version is fine just acknowledging.
There was a problem hiding this comment.
oh interesting. So we're throwing away the 3-state visible/occluded/invisible in favor of visible true/false? (looking at the green vs red in the above diff)
Does sort of seem like we want to just pass the enum through yeah? Otherwise no point in having the 3 different states in the editor?
There's value in having visibility = full mean normally on screen, occluded mean in frame but partially or fully blocked by something else (user's choice). and visibility = none can mean fully blocked or off screen (user's choice -- as long as their consistent).
If I'm missing context here, do correct but otherwise prefer we represent the 3-states absent a strong argument to not do so. nice flag jim.
There was a problem hiding this comment.
This particular line that I've responded too doesn't give full context as to what the change is.
But basically it's:
visibility: Enum vs: (visible: bool, occluded: bool, invisible: bool)
where the underlying JSON has: the latter but it may be better encapsulated as the former.
I wouldn't worry too much Justin.
There was a problem hiding this comment.
I think a string enum is definitely preferable to 3 fields. However we're technically not "throwing away" the enum since it is here but (as far as I can see) was always None. At the moment we have:
- label row export, JSON -> 3 fields (visible/occluded/invisible)
- label row export, COCO -> "enum" 0/1/2
- SDK export -> didn't work; deciding in this PR
Given that it's already not consistent, maybe we can lean towards the enum here
arthur-encord
force-pushed
the
av/create_skeletoncoordinates_instance
branch
from
73702b5 to
008e6b4
Compare
Adding SkeletonCoordinates object parser to work with the INVISIBLE/ OCCLUDED in SkeletonCoordinate.