Skip to content

Cleanup conversion transforms #6801

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

Merged
merged 13 commits into from
Oct 21, 2022
Merged

Cleanup conversion transforms #6801

merged 13 commits into from
Oct 21, 2022

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Oct 20, 2022

This PR stems from the discussion in #6783 (comment). @datumbox and I agreed to the following things:

  • Let's not have a copy flag on our convert_* kernels. It is currently unused for convert_color_space. It is used extensively for convert_bounding_box in our kernels due the idiom of converting to a specific format, performing the computation, and converting back. In the future, we wanted to have a look at these kernels anyway for possible performance gains by implementing the computation for every format. After this PR the current implementation like

    bounding_box = convert_format_bounding_box(
        bounding_box, old_format=format, new_format=features.BoundingBoxFormat.XYXY
    )

    changes to

    bounding_box = (
        bounding_box.clone()
        if format == features.BoundingBoxFormat.XYXY
        else convert_format_bounding_box(bounding_box, old_format=format, new_format=features.BoundingBoxFormat.XYXY)
    )
  • Let's not have any .to_*() methods on the features. While useful for debugging, they go against the other idioms we are using. We can always re-add them later if there is user demand for it. Note that Label.to_categories() is an exception since it has no transformation associated with it.

cc @vfdev-5 @datumbox @bjuncek

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 20, 2022

bounding_box = (
    bounding_box.clone()
    if format == features.BoundingBoxFormat.XYXY
    else convert_format_bounding_box(bounding_box, old_format=format, new_format=features.BoundingBoxFormat.XYXY)
)

IMO, this is too bulky. I do not mind to have copy in convert_format_bounding_box and reduce lines of user's code...

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.

Thanks @pmeier, looks good overall. Just a couple of minor comments.

@pmeier
Copy link
Collaborator Author

pmeier commented Oct 20, 2022

@vfdev-5

I do not mind to have copy in convert_format_bounding_box and reduce lines of user's code...

Same here with the focus on consistency. If one convert_* kernel or dispatcher has a copy flag, every should have it. Currently we have copy: bool = True, which has a runtime implication unless someone explicitly sets the copy=False in case no copy is needed. Meaning convert_dtype of #6783 (comment) will be slower by default than before for mostly no reason. This PR basically hard-codes copy=False.

Vasilis argument was that besides for convert_format_bounding_box we aren't using the copy flag anywhere. Even that is only used a handful of times throughout our API. Thus, if we want consistency, it is fairly simple to remove and avoids dead code. I don't want to misrepresent the argument, so I let him chime in here. cc @datumbox

I'm ok with or without the copy flag if we are consistent with it. I'll let the two of you hash it out.

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.

LGTM, only one comment below.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 21, 2022

@datumbox @pmeier can we do the following:

  • make all conversion ops like _xywh_to_xyxy inplace (without cloning internally) and
  • we can apply .clone() for the output of convert_format_bounding_box
bounding_box = convert_format_bounding_box(
    bounding_box.clone(), old_format=format, new_format=features.BoundingBoxFormat.XYXY
)

?

@datumbox
Copy link
Contributor

@vfdev-5 that would make our only operator that does in-place ops (outside of the optimizations that have no effect). If we were to go down that route, purely for speed reasons, I would recommend adopting the inplace argument for this one kernel (because it's used so often in conversions). The default should be inplace=False in that case. I would do this outside of this PR though after measuring its speed.

Conflicts:
	torchvision/prototype/features/_image.py
	torchvision/prototype/features/_video.py
@pmeier pmeier merged commit 121a780 into pytorch:main Oct 21, 2022
@pmeier pmeier deleted the convert-to branch October 21, 2022 12:34
facebook-github-bot pushed a commit that referenced this pull request Oct 27, 2022
Summary:
* remove copy from convert_color_space

* remove copy from convert_format_bounding_box

* remove .to_* methods from features

* remove unnecessary clones

* add perf todos

* refactor convert_color_space

* lint

* remove another clone

* and another clone

* remove a missed copy

Reviewed By: YosuaMichael

Differential Revision: D40722906

fbshipit-source-id: 3b757af1b69fcd85b085a5df9ff88cdaeafca130
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.

4 participants