-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[NOMERGE] Feedback Transforms API implementation #5375
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
Conversation
💊 CI failures summary and remediationsAs of commit dc7d0d2 (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
Job | Step | Action |
---|---|---|
Lint Python code and config files | 🔁 rerun |
This comment was automatically generated by Dr. CI (expand for details).
Please report bugs/suggestions to the (internal) Dr. CI Users group.
return convert_bounding_box_format( | ||
torch.stack((x, y, w, h), dim=-1), | ||
old_format=BoundingBoxFormat.XYWH, | ||
new_format=BoundingBoxFormat.XYXY, | ||
) | ||
|
||
|
||
_resize_image = _F.resize | ||
_resize_image = _F.resize # How about videos? The low level transfroms supports it |
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.
Right now there is no custom feature type for videos and the tentatively plan is to treat videos as Image
's with an extra batch dimensions cc @bjuncek. Since the kernel should be agnostic to batch dimensions, they are automatically supported by all image transforms.
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.
We should finalize this plan soon. We got outstanding work with Videos so we need to be able to have an idea of how this works for them. Once the plan is finalized, we will be able to parallelize the work of porting all transforms concurrently.
I agree about the underlying kernels being agnostic, sounds good.
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.
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.
Annoyingly, I still haven't gotten a reply from any of them.
@datumbox , maybe you could help me offline in order to make this discussion happen?
tentatively plan is to treat videos as Image's with an extra batch dimensions
Is there a downside to creating a video
feature class that for the time being implement just this, but would be possible to extend with stuff like audio or cc-data or encoded bytes if support for this will be needed?
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.
@bjuncek ping me offline with the names of the people you try to talk to to see if I can help.
When we say "extra branch dimensions", could you clarify the proposal?
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 don't think there is real downside other than being more verbose in our transforms implementations. For example, we would need to have resize_video
that simply defers to resize_image
. This has to happen for every single image / video kernel.
Spoke offline with @pmeier and I'm going to close this. The comments will be addressed in a follow up PR. |
No description provided.