Skip to content

Rename convert_bounding_box_format => convert_format_bounding_box #6582

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 3 commits into from
Sep 14, 2022

Conversation

datumbox
Copy link
Contributor

Fully automatic update using my IDE. There shouldn't be anything worth reviewing. Should be fine if the tests pass.

The rational on the rename is that this is low-level Bounding Box kernel and as every such kernel, it should end with _bounding_box. I spotted it while refactoring the tests to catch non JIT-scriptable midlevels.

If my understanding is incorrect and we consider this a midlevel, feel free to close the PR.

@oke-aditya
Copy link
Contributor

Sounds bit bad grammatically convert_format_bounding_box :( and convert_bounding_box_format was more grammatically correct.

@datumbox
Copy link
Contributor Author

datumbox commented Sep 14, 2022

@oke-aditya Though I agree that convert_bounding_box_format is the natural way to describe what the method does, it conflicts with literally all other naming conventions such as horizontal_flip_bounding_box, resized_crop_bounding_box, etc. It also makes it much harder to gather all the low-level kernels. See the tests at #6584 to see what I mean.

Not merging this is not going to be the end of the world, but this method is literally the only one that doesn't follow the naming convention and might require custom tricks to retrieve.

@oke-aditya
Copy link
Contributor

Yes. We can take the poetic license here I think 🤔

@oke-aditya
Copy link
Contributor

I mean it's OK let's rename.

@datumbox
Copy link
Contributor Author

Yes. We can take the poetic license here I think

I like that. Very poetic. 😄

I'll leave it to Victor and Philip to decide. No strong opinions on my side. :)

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

This is the same change that we made with convert_image_color_space_{tensor, pil} -> convert_color_space_image_{tensor, pil} in #6408.

def convert_color_space_image_tensor(

This weird naming was somewhat mitigated by introducing a dispatcher convert_color_space. Within in our current rules to need at least two kernels before we add a dispatcher, that is not possible here. Still, I agree, we should do this for consistency.

Thanks Vasilis. LGTM if CI is green!

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Sep 14, 2022

I do not have a strong opinion on this rename, but yes, it reads less natural

@datumbox datumbox merged commit 0874338 into pytorch:main Sep 14, 2022
@datumbox datumbox deleted the rename/convert_format_bounding_box branch September 14, 2022 22:07
facebook-github-bot pushed a commit that referenced this pull request Sep 15, 2022
…ding_box` (#6582)

Summary:
* Rename `convert_bounding_box_format` => `convert_format_bounding_box`

* Add missed replacement.

Reviewed By: jdsgomes

Differential Revision: D39543284

fbshipit-source-id: 1b239d7e959e0ac78034d3854b8af6c4c77828a8
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.

5 participants