Skip to content

[proto] Draft for Transforms API v2 #6205

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

Closed
wants to merge 28 commits into from

Conversation

vfdev-5
Copy link
Collaborator

@vfdev-5 vfdev-5 commented Jun 27, 2022

A draft for Transforms API v2

vfdev-5 added 22 commits June 22, 2022 10:03
- Added H/V flip ops
- Added resize op
- Added center_crop
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking cool so far. Just a few questions:

Comment on lines 20 to 22
# Just output itself
# How dangerous to do this instead of raising an error ?
return self
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would raise a "not implemented" error here and implement no-op explicitly on the implementations. So if bbox can't blur, then it should explicitly implement to be no-op as opposed to leaving the default implementation which returns self. Could help us avoid issues but if you disagree I'm happy to leave as-is and discuss later.

Copy link
Collaborator Author

@vfdev-5 vfdev-5 Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that raising NotImplemented could be good. The only drawback I found that all targets features like Label, OneHotLabel should also implement explicitly these no-ops. From the first glance, implementing all methods like OneHotLabel.blur(self, *args, **kwargs) as no-op made me think that it is a bit too much...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you. I've added this for your consideration. Feel free to ignore or postpone for later.

@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Jul 25, 2022

Closed in favor of #6305

@vfdev-5 vfdev-5 closed this Jul 25, 2022
@vfdev-5 vfdev-5 deleted the proto-transforms-api-oo branch July 25, 2022 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants