Skip to content

Improve DETR post-processing methods#19205

Merged
alaradirik merged 9 commits intohuggingface:mainfrom
alaradirik:detr-featext-fix
Sep 29, 2022
Merged

Improve DETR post-processing methods#19205
alaradirik merged 9 commits intohuggingface:mainfrom
alaradirik:detr-featext-fix

Conversation

@alaradirik
Copy link
Contributor

What does this PR do?

  • Adds post_process_object_detection, post_process_semantic_segmentation, post_process_instance_segmentation and post_process_panoptic_segmentation methods with optional resizing and thresholding to filter out low probability predictions.

This PR is part of a larger effort to ensure post-processing methods have consistent naming, input arguments and output. Deprecation warnings are added to existing post-processing methods.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • [ X] Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • [ X] Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 26, 2022

The documentation is not available anymore as the PR was closed or merged.

@alaradirik alaradirik requested a review from sgugger September 28, 2022 12:15
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. As mentioned internally, don't hesitate to separate PRs in focused bits: here there could have been:

  • one PR to quickly fix the comments
  • one PR to add the post processing to DETR
  • one PR for the deprecation warnings

Also wondering if we should target a lower version than v5 for the removal of the methods, since this is all a bit of an experimental API.

@alaradirik
Copy link
Contributor Author

@amyeroberts @sgugger thank you both for the feedback! Could you take a final look at the PR? If everything looks fine, I'll apply the same changes to MaskFormer and update the model cards.

@alaradirik
Copy link
Contributor Author

Thanks for working on this. As mentioned internally, don't hesitate to separate PRs in focused bits: here there could have been:

  • one PR to quickly fix the comments
  • one PR to add the post processing to DETR
  • one PR for the deprecation warnings

Makes sense, I'll split my future PRs into smaller, issue-specific ones.

Also wondering if we should target a lower version than v5 for the removal of the methods, since this is all a bit of an experimental API.

I agree, we can target a lower version for the removal of deprecated methods.

Copy link
Contributor

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks for the PR and work on improving this

Left a few nits, main comment is about moving the processing helper functions out of the class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants