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

Group comparison view: update boxplot info in clinical tab #4924

Merged
merged 4 commits into from
Dec 2, 2024

Conversation

TJMKuijpers
Copy link
Contributor

In the group comparison view, when we go to the clinical tab there is the option to show different clinical attributes per group. Those clinical attributes are either visualized as a stacked bar chart or boxplot.

The figure with the boxplot shows a boxplot with all samples as scatter points.
image

When we set the visualization type to table, we can see that only the boxplot information is shown:
image

This is quite confusing for the user since the minimum and maximum values are the lower and upper whisker of the boxplot, not of the data points. We should add this information but also add to the current table that these numbers are describing the boxplot.

This would be the proposed solution (image created with test study es_0)
image

Copy link

netlify bot commented Jun 14, 2024

Deploy Preview for cbioportalfrontend ready!

Name Link
🔨 Latest commit 9ec42f5
🔍 Latest deploy log https://app.netlify.com/sites/cbioportalfrontend/deploys/674dd03cb290cf00089ed522
😎 Deploy Preview https://deploy-preview-4924.cancerrevue.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@TJMKuijpers TJMKuijpers changed the title Update the table with scatter data summary Group comparison view: update boxplot info in clinical tab Jun 14, 2024
@TJMKuijpers TJMKuijpers marked this pull request as ready for review June 17, 2024 14:02
@TJMKuijpers TJMKuijpers requested review from inodb and dippindots June 17, 2024 14:03
@TJMKuijpers
Copy link
Contributor Author

@inodb in the third image: I'm not sure what the standard is --> should boxplot (and scatterplot) be one or two words. It seems that both are used when I do a quick google search.

@inodb
Copy link
Member

inodb commented Jun 19, 2024

@TJMKuijpers Good catch! Thanks for trying to come up with a solution to this

I think the "Table" name here is unintuitive. How about we call it "Summary Statistics"?

In the "Summary Statistics" maybe better to not put the words 'boxplot' nor 'scatterplot'. E.g. imagine sharing a link to this table directly. It's unclear what's meant by scatterplot/boxplot. The boxplot stats are already shown as a tooltip on the boxplot itself. It might be better to only show summary statistics similar to e.g. what the R package does here: https://www.reneshbedre.com/blog/describe-function-in-r.html. So just general data stats (i.e. the scatterplot table) and maybe a few others that the R package reports

Does that make sense?

@TJMKuijpers
Copy link
Contributor Author

@inodb I updated the table to match the descriptive information Pandas or R returns:
image

Table will be updated when the user clicks on "log scale':
image
image

@TJMKuijpers
Copy link
Contributor Author

@inodb and @dippindots could you have a look at this PR?

Copy link
Member

@dippindots dippindots left a comment

Choose a reason for hiding this comment

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

@TJMKuijpers Overall it looks good to me codewise, just had a few comments about the type, it seems like we have more than one any type, and we should try to create specific types for them.

src/pages/groupComparison/SummaryStatisticsTable.tsx Outdated Show resolved Hide resolved
src/pages/groupComparison/SummaryStatisticsTable.tsx Outdated Show resolved Hide resolved
src/shared/components/plots/BoxScatterPlot.tsx Outdated Show resolved Hide resolved
src/shared/components/plots/BoxScatterPlot.tsx Outdated Show resolved Hide resolved
Copy link
Member

@dippindots dippindots 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 updating those types, they look great now. I just have one more comment on this, then we also need @inodb to sign off from the product side

src/shared/components/plots/BoxScatterPlot.tsx Outdated Show resolved Hide resolved
@TJMKuijpers TJMKuijpers force-pushed the scatter-statistics-table branch 3 times, most recently from fce176c to 14f05b0 Compare July 12, 2024 07:25
Copy link
Member

@dippindots dippindots left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@inodb inodb left a comment

Choose a reason for hiding this comment

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

Thanks so much @TJMKuijpers !

This looks great!

image

Few thoughts:

  • MAD - should we spell this one out like Mean Absolute Deviation (MAD)?
  • "Lower/higher whisker" - whisker looks odd without seeing the chart. Do we need those? Maybe better to remove? The whisker values show up in the a tooltip of the chart itself on mouseover, so not sure if we need them for the table?

@TJMKuijpers TJMKuijpers force-pushed the scatter-statistics-table branch from 0639e54 to dbcb033 Compare July 18, 2024 14:01
Copy link
Member

@inodb inodb left a comment

Choose a reason for hiding this comment

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

LGTM - thank you!

Update table with log scale

Define types in data table

Update screenshot

Remove whisker info and add full name MAD

Update screenshot
@TJMKuijpers TJMKuijpers force-pushed the scatter-statistics-table branch from 81f836f to a1eeed7 Compare September 11, 2024 15:22
@inodb inodb merged commit 209c834 into cBioPortal:master Dec 2, 2024
5 of 7 checks passed
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.

3 participants