Skip to content
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

Add Modified Panoptic Quality metric #1591

Closed
marcocaccin opened this issue Mar 5, 2023 · 5 comments · Fixed by #1627
Closed

Add Modified Panoptic Quality metric #1591

marcocaccin opened this issue Mar 5, 2023 · 5 comments · Fixed by #1627
Labels
enhancement New feature or request
Milestone

Comments

@marcocaccin
Copy link
Contributor

🚀 Feature

Add Modified Panoptic Quality metric introduced in Seamless Scene Segmentation, Section 4 "Revisiting Panoptic Segmentation".

Motivation

The Modified Panoptic Quality is a subtle modification of the original, and it comes in handy to have it available when dealing with Panoptic Segmentation problems: it's not easy to say whether the original or the modified one is a better choice in any given situation, so one would ideally track both.
To prove the point, some public datasets such as nuScenes use both variants for evaluation on the leaderboard.

Pitch

I'm happy to continue the work on PQ to extend its functionality 😉

Alternatives

Additional context

Question to the maintainers: would you rather see this implemented in its own class and functional versions, or as a switch (e.g., implementation: Literal['original', 'modified'] = 'original') of the already existing metrics?
The code is almost identical besides a couple of extra if statements in the update() step, and so are the signatures, so I would lean on the latter option to avoid additional copy-paste.

@marcocaccin marcocaccin added the enhancement New feature or request label Mar 5, 2023
@SkafteNicki
Copy link
Member

Hi @marcocaccin, sound like a great enhancement.
After looking through the paper you linked, it looks to me that the easier solution would be to just have this as an argument implementation: Literal['original', 'modified'] = 'original' in the current PQ implementation as the authors does not really consider this a new metric but only an adjusted version of the original.

@stancld
Copy link
Contributor

stancld commented Mar 6, 2023

@SkafteNicki Just my two cents. On the other hand, we have BLEUScore and SacreBLEUScore, which basically differ in tokenization. But both options seem to be acceptable.

@SkafteNicki
Copy link
Member

@stancld true. To me, it actually does not matter what option we go with, like you said but options are acceptable :]

@marcocaccin
Copy link
Contributor Author

marcocaccin commented Mar 6, 2023

Thanks for the feedback! After thinking about it, maybe the least confusing solution is to have a common private module where all the functionality is implemented (torchmetrics/functional/detection/_panoptic_quality_common.py) and separate super-shallow modules containing the individual functional and class implementations.
The only reason I have for that is the docstring: I find that linking a single paper and writing a single docstring test is the least confusing.
Feel free to assign the issue to me btw 🙂

ps: first time I come across the Sacre bleu score, that was a good chuckle

@SkafteNicki SkafteNicki added this to the future milestone Mar 7, 2023
@marcocaccin
Copy link
Contributor Author

For a more concrete example, this is how the feature would shape up when having this feature as a separate metric: marcocaccin#1 (it's on my fork to avoid triggering CI for nothing).
Now that I wrote it (almost, missing all new tests), I have doubts whether it's the right way to go because of the amount of duplication. Let me know if the Literal argument is more or less desirable now

@Borda Borda modified the milestones: future, v1.0.0 Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants