Skip to content

Remove the unused/buggy --train-center-crop flag from Classification preset #6642

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 4 commits into from
Sep 26, 2022

Conversation

datumbox
Copy link
Contributor

@datumbox datumbox commented Sep 25, 2022

NicolasHug
NicolasHug previously approved these changes Sep 26, 2022
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

LGTM but since we use the bug label for release notes and this isn't a released bug, I'd suggest to remove the label to avoid confusion

@datumbox datumbox changed the title Fixing inverted center_crop check on Classification preset Remove the unused/buggy --train-center-crop flag from Classification preset Sep 26, 2022
@NicolasHug NicolasHug dismissed their stale review September 26, 2022 09:58

New changes since approval

Copy link
Contributor

@TeodorPoncu TeodorPoncu left a comment

Choose a reason for hiding this comment

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

Looks good to me, this should replicate 1:1 the setting in which the model was trained.

@datumbox
Copy link
Contributor Author

@TeodorPoncu Thanks for the review and for providing the details to verify how the model was trained. I'll add a note to to the checkpoint area to clarify that the log incorrectly reports the training argument and that the correct command is fixed in this PR. On my side, I also independently verified that --train-center-crop wasn't used for this model by looking back on each commit prior training.

@NicolasHug I think the tag discussion was brought up before. I understand that by tagging this as a bug, our automated script that prepares the release notes will put it under the bugs section and that requires manual cleaning (cc @YosuaMichael who is the POC of the upcoming release). One issue is that the bug tag is used to track how many bugs we introduce and solve overall. This has implications on tracking the dashboards that TorchVision maintains, the Better Engineering work, the upstream Core breakages and the classification of cherrypicks of the release (which again might be introduced only on development and might not pre-exist on previous releases). Perhaps we can avoid this by introducing an additional tag to filter these out? If you feel strongly about the tag in this PR, feel free to remove it and we can discuss it on a future weekly.

@datumbox datumbox merged commit a35be97 into pytorch:main Sep 26, 2022
@datumbox datumbox deleted the bugfix/references_center_crop branch September 26, 2022 13:13
facebook-github-bot pushed a commit that referenced this pull request Sep 29, 2022
…sification preset (#6642)

Summary:
* Fixing inverted center_crop check on Classification preset

* Remove the `--train-center-crop` flag.

Reviewed By: YosuaMichael

Differential Revision: D39885428

fbshipit-source-id: 0ac6b72d55b05f014d690ac51645cdbd3f29d2b9
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