-
Notifications
You must be signed in to change notification settings - Fork 373
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
Add default value for info
argument of ClassDescription
#3017
Conversation
* Closes #2924 This took me over an hour to figure out 😭
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.
not acutally sure if the default value is a good idea. "Give a title to this class" becomes odd when one doesn't specify which class is meant - or am I missunderstanding the situation?
I don't know - Where are you quoting from? I can't find "Give a title to this class" anywhere |
Oh that's not a quote, that's more of my interpretation of what giving a setting an annotationinfo means |
I don't like the implicit default value either. I think I'll merge this and cherry-pick for 0.8.1 (since it fixes a subtle 0.8.0 API regression) and then make a new PR to make |
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.
Probably worth adding a comment to the generated docstring (in class_description.fbs
) that if no info is provided, the default is 0.
The consequences of this really are that a line such as:
rr.ClassDescription(keypoint_connections=[(0, 1), (1, 2), (2, 0), (2, 3), (0, 4), (1, 5)])
Implicitly says to annotate keypoint_connections for keypoints that have a class_id of 0, which matches the behavior of log_points
, which sets the class_id to 0 if you log keypoints without logging a class-id.
@@ -29,7 +29,8 @@ | |||
|
|||
def classdescription_init( | |||
self: ClassDescription, | |||
info: AnnotationInfoLike, | |||
info: AnnotationInfoLike = 0, |
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 still strikes me as a bit of a footgun, but it matches the pre-existing behavior.
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'm gonna remove it in a follow-up-PR, but keep it here to not break API further in the 0.8.1 patch release
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.
If you want to break it in the future, why not break it now? Is there a meaningful difference between breaking it in 0.8 and breaking it in 0.9?
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.
It's more that I want to do the least intrusive change for 0.8.1.
If we require info
to be explicit, should we also make it explicit for log_points
? Why require explicitness in one but not the other?
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.
Fair enough.
face_tracking
example fails with newAnnotationContext
implementation #2924Two changes:
info
without having to also specify label and color, usinginfo=0
.id=0
, matching 0.7.0 behavior and howlog_points
work(This took me over an hour to figure out 😭)
Checklist