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

Heatmap Support for Object Detection #2303

Merged
merged 2 commits into from
Aug 31, 2023
Merged

Heatmap Support for Object Detection #2303

merged 2 commits into from
Aug 31, 2023

Conversation

Advitya17
Copy link
Collaborator

@Advitya17 Advitya17 commented Aug 30, 2023

This PR adds heatmap support for OD scenario to achieve corresponding feature parity with other dashboards.

Description

image

image

The main drawback of this support is I had to end up disabling the magenta color during N/A status while metrics are being computed, because I could not override the metrics to have transparent color anywhere in StatsTableUtils.ts as that ended up overriding the heatmap colors.

image

Considering the magenta color with N/A display is only valid for OD & QA scenarios, and is not a breaking feature change but a nice-to-have UI (not to mention potentially switching this with async loaders), this support can be enabled in a future PR.

Checklist

  • I have added screenshots above for all UI changes.
  • I have added e2e tests for all UI changes.
  • Documentation was updated if it was needed.

Copy link
Contributor

@romanlutz romanlutz left a comment

Choose a reason for hiding this comment

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

This passes information (modelType) quite deep into the logic that doesn't really need this information. Instead, would it be possible to set this at the top level (constructor of ModelOverview, for example) based on the model type (technically, task type, but I guess it's fine...) and factor it into useTexturedBackgroundForNaN?

In general, we want to keep the components and their associated logic as decoupled as possible, and model type is rather the opposite. It is entirely possible that I'm missing something here that makes this necessary, so feel free to point that out!

@Advitya17
Copy link
Collaborator Author

Advitya17 commented Aug 31, 2023

This passes information (modelType) quite deep into the logic that doesn't really need this information. Instead, would it be possible to set this at the top level (constructor of ModelOverview, for example) based on the model type (technically, task type, but I guess it's fine...) and factor it into useTexturedBackgroundForNaN?

In general, we want to keep the components and their associated logic as decoupled as possible, and model type is rather the opposite. It is entirely possible that I'm missing something here that makes this necessary, so feel free to point that out!

@romanlutz Thanks for the question. Unfortunately the way the code is setup, if I just pass the ModelType to the useTexturedBackgroundForNaN which exactly takes as input this.state.showHeatmapColors, it totally removes the blue heatmap colors.

image

With the way the code flow works with N/A magenta colorValue and the blue color rendering for the heatmap (and the same input that both take in for assigning colors - this.state.showHeatmapColors), it only seemed possible to disable the N/A magenta color assignment inside the function for OD & QA, so that the blue color rendering isn't disabled as well.

@romanlutz
Copy link
Contributor

Oh. I guess I had assumed it's straightforward, sorry. If you see a simple way to refactor it go ahead, otherwise ignore.

@Advitya17 Advitya17 merged commit f6cd1f8 into main Aug 31, 2023
65 checks passed
@Advitya17 Advitya17 deleted the agemawat/od_heatmap branch August 31, 2023 20:48
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.

2 participants