Add Stacked Bar Chart and sorting to Plots Tab #4960

SURAJ-SHARMA27 · 2024-08-06T17:23:48Z

Describe changes proposed in this pull request:
Addition of Sort By MinorCategory Options:

Implemented a new feature allowing users to sort by minorCategory.
Added a dropdown menu with minorCategory options for sorting.

Sorted Stacks by Selected MinorCategory:

For the selected option from the dropdown, entities will appear at the beginning of each stack in a sorted order with respect to the count of chosen minorCategory.

Any screenshots or GIFs?

screen-capture.9.webm

Notify reviewers

@alisman
@sowmiyaa-kumar
@zeynepkaragoz
@TJMKuijpers
@inodb

netlify · 2024-08-06T17:24:04Z

✅ Deploy Preview for cbioportalfrontend ready!

Name	Link
🔨 Latest commit	`bc6e5b9`
🔍 Latest deploy log	https://app.netlify.com/sites/cbioportalfrontend/deploys/66bb9d27cde1bf0008050021
😎 Deploy Preview	https://deploy-preview-4960--cbioportalfrontend.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

inodb · 2024-08-07T14:25:24Z

@SURAJ-SHARMA27 this looks awesome - thank you!

For the sorting, can we also sort by "# samples"? The value of the Y-axis basically

This would be similar to e.g. how it works here for treatment response:

This is slightly separate, but maybe for treatment response, the sorting should be using your new "Sort By" component too

schultzn · 2024-08-07T14:48:54Z

Looks great. It would be nice to also be able to sort by the overall number of samples (I see that comment above) and also to change the sort order back to alphabetical.

jjgao · 2024-08-07T15:23:46Z

Good work @SURAJ-SHARMA27 !

Somehow x-axis labels does not change - always alphabetically, but the bars are changing

When switch Plot Type, should we change the sort value, ie. sort by number of samples if it's "Stacked bar chart" and by % samples if it's "100% stacked bar chart"?
Should the 'Sort By' menu be included in the 'Vertical Axis' section since it's using the values from vertical axis.
It would be nice to add the sort by parameter to the url.

SURAJ-SHARMA27 · 2024-08-07T15:51:09Z

Good work @SURAJ-SHARMA27 !

Somehow x-axis labels does not change - always alphabetically, but the bars are changing

When switch Plot Type, should we change the sort value, ie. sort by number of samples if it's "Stacked bar chart" and by % samples if it's "100% stacked bar chart"?

Should the 'Sort By' menu be included in the 'Vertical Axis' section since it's using the values from vertical axis.

It would be nice to add the sort by parameter to the url.

Actually, I implemented that earlier, as you can see in the video I attached at the time of the PR. The labels were changing, but I forgot to include the logic when pushing it. Thank you for pointing that out. I have now made the changes and pushed it.

SURAJ-SHARMA27 · 2024-08-07T18:53:23Z

I think I have implemented most of the suggestions and added two more options for sorting: sort by the number of samples and alphabetically. If you could review it. @inodb

screen-capture.14.webm

inodb · 2024-08-08T12:56:08Z

Amazing - nice work @SURAJ-SHARMA27 !

Should the 'Sort By' menu be included in the 'Vertical Axis' section since it's using the values from vertical axis.

@jjgao Good question - I kinda like it separate from x/y axis selection since we might also want to allow sorting by multiple values in the future (which could be like a multi-select dropdown element)

Few more thoughts:

Should we rename # samples to Number of Samples? I like the consistency with the current y-axis and we use # in several places, but it's maybe not as obvious in the sort selection what # means?
Can we make Alphabetical show up as the default in the sort by? It currently shows an empty selection on first load:

gblaih · 2024-08-08T17:15:23Z

src/pages/groupComparison/MultipleCategoryBarPlot.tsx

@@ -58,6 +58,11 @@ export interface IMultipleCategoryBarPlotProps {
    svgRef?: (svgContainer: SVGElement | null) => void;
    pValue: number | null;
    qValue: number | null;
+    SortByDropDownOptions?: { value: string; label: string }[];


sortByDropDownOptions

gblaih · 2024-08-08T17:15:28Z

src/pages/groupComparison/MultipleCategoryBarPlot.tsx

@@ -425,6 +430,43 @@ export default class MultipleCategoryBarPlot extends React.Component<

    @computed get labels() {
        if (this.data.length > 0) {
+            if (this.props.sortByOption == 'SortByTotalSum') {


gblaih · 2024-08-08T17:15:37Z

src/pages/groupComparison/MultipleCategoryBarPlot.tsx

+                );
+                return sortedMajorCategories;
+            } else if (
+                this.props.sortByOption != '' &&


lets use === and !== throughout the code

gblaih · 2024-08-08T17:15:48Z

src/shared/components/plots/PlotsTab.tsx

+                                        this?.SortByDropDownOptions
+                                    }
+                                    updateDropDownOptions={
+                                        this?.updateDropDownOptions
+                                    }
+                                    sortByOption={this?.sortByOption}


is this?. necessary? i'd think this. is fine

gblaih · 2024-08-08T17:18:50Z

src/pages/groupComparison/MultipleCategoryBarPlot.tsx

+            if (this.props.sortByOption == 'SortByTotalSum') {
+                const majorCategoryCounts: any = {};
+
+                this.data.forEach(item => {


this code looks pretty similar to the the function sortDataByOption you made below. it would be nice to extract this into a function that can be reused

gblaih · 2024-08-08T17:21:45Z

src/shared/components/plots/MultipleCategoryBarPlotUtils.ts

+            (a, b) => majorCategoryCounts[b] - majorCategoryCounts[a]
+        );
+
+        const reorderCounts = (counts: any) => {


instead of making this function inside, lets extract this into a new function outside for clarity and reuse

SURAJ-SHARMA27 · 2024-08-09T05:30:30Z

I have implemented all the changes as suggested. You can review them. I am also attaching a video for reference.
@inodb @gblaih

screen-capture.15.webm

inodb · 2024-08-13T14:30:47Z

@SURAJ-SHARMA27 Thanks for the fixes! I noticed the "Sort by" option is currently missing here, do you see that?

alisman · 2024-08-13T14:49:22Z

src/pages/groupComparison/MultipleCategoryBarPlot.tsx

@@ -435,6 +451,18 @@ export default class MultipleCategoryBarPlot extends React.Component<
        }
    }

+    private setInitialSelectedOption = () => {


this is a bit of an anti-pattern. calling from the child component to update the state of parent component on render. it should be possible to do this in parent component prior to mount?

In the parent component, it doesn't contain data in the form of IMultipleCategoryBarPlotData, from which I have to extract all the unique major categories for dropdown options. Therefore, to avoid extra calculations, I passed the prop from the parent and updated it when it became available in the child.

alisman · 2024-08-13T14:50:03Z

src/shared/components/plots/MultipleCategoryBarPlotUtils.ts

@@ -98,6 +98,71 @@ export function sortDataByCategory<D>(
        }
    });
 }
+export function getSortedMajorCategories(


can we have a comment explaining what is meant by "Major Category" ?

The data is defined in a way that it contains x-axis labels referred to as major categories. This functionality is used twice: once during the sorting of data and again when the getLabels function in MultipleCategoryBarPlot.tsx is called. Therefore, I created a separate function for that to enhance reusability.

alisman · 2024-08-13T14:50:32Z

src/shared/components/plots/MultipleCategoryBarPlotUtils.ts

+    sortByOption: string | undefined
+): string[] {
+    if (sortByOption === 'SortByTotalSum') {
+        const majorCategoryCounts: any = {};


avoid 'any'. lets type this.

Yes, I have defined the types across all the changes.

alisman · 2024-08-13T14:51:30Z

src/shared/components/plots/MultipleCategoryBarPlotUtils.ts

+    if (sortByOption === 'SortByTotalSum') {
+        const majorCategoryCounts: any = {};
+
+        data.forEach(item => {


i think you can do this operation with flatMap, which will avoid nesting forEach (which is confusing)

The above function is used to find the sum of each majorCategory for different minorCategories. For instance, consider the following example:

data contains three minorCategory names: Biopsy, Resection, and Cusa. Each of these minorCategories contains a large array named counts, and each element in that array is an object containing majorCategory, count, and percentage. For example, for Biopsy, the majorCategories might include Germ Cell Tumor and Thyroid Cancer....

I am maintaining a totalSum of these majorCategories and then applying sorting.

If I use flatMap, it would first be difficult to determine which majorCategory belongs to which minorCategory. Additionally, I think it would require more space and have an equivalent time complexity of O(n * m) as it does now with the two nested loops. First, I would need to perform the flatMap operation (a map operation followed by a flattening operation, essentially equivalent to a nested loop) and then use reduce to accumulate the counts. What are your suggestions should i follow another approach?

alisman · 2024-08-13T14:52:13Z

src/shared/components/plots/MultipleCategoryBarPlotUtils.ts

+
+            return sortedEntityData.counts.map(item => item.majorCategory);
+        }
+    }


just complete the else here.

yes completed

alisman · 2024-08-13T14:53:09Z

src/shared/components/plots/MultipleCategoryBarPlotUtils.ts

+    data: IMultipleCategoryBarPlotData[],
+    sortByOption: string | undefined
+): string[] {
+    if (sortByOption === 'SortByTotalSum') {


see if SortByTotalSum is available on an enum somewhere. seems likely that it is

No, it was not there; I have defined an enumeration and used it

alisman · 2024-08-13T14:53:57Z

src/shared/components/plots/MultipleCategoryBarPlotUtils.ts

+        return Object.keys(majorCategoryCounts).sort(
+            (a, b) => majorCategoryCounts[b] - majorCategoryCounts[a]
+        );
+    } else if (sortByOption !== '' && sortByOption !== 'alphabetically') {


use constant for 'alphabetically' or see if it's available on an enum

I have defined an enumeration and used it

alisman · 2024-08-13T14:54:54Z

src/shared/components/plots/MultipleCategoryBarPlotUtils.ts

+    const sortedMajorCategories = getSortedMajorCategories(data, sortByOption);
+
+    if (sortByOption === 'SortByTotalSum' || sortedMajorCategories.length > 0) {
+        const reorderCounts = (counts: any) => {


avoid 'any'

Yes, I have defined the types across all the changes.

inodb · 2024-08-13T16:45:26Z

@SURAJ-SHARMA27 Just discussed this during the community call, people liked it! Some more feedback from the group:

When switching to percentage, the bars should sort by the percentage value (rather than absolute value)
On the Table: maybe disable the sort here? Or if it is easy to enable sorting there too, enable it.
Remove "Sort By" on charts where it is not functional (e.g. continuous vs continuous). Currently, the "Sort by" seems to show old values from a previous selection

SURAJ-SHARMA27 · 2024-08-13T16:57:44Z

@SURAJ-SHARMA27 Thanks for the fixes! I noticed the "Sort by" option is currently missing here, do you see that?

yes i fixed it

SURAJ-SHARMA27 · 2024-08-13T17:02:57Z

@SURAJ-SHARMA27 Just discussed this during the community call, people liked it! Some more feedback from the group:

When switching to percentage, the bars should sort by the percentage value (rather than absolute value)

On the Table: maybe disable the sort here? Or if it is easy to enable sorting there too, enable it.

Remove "Sort By" on charts where it is not functional (e.g. continuous vs continuous). Currently, the "Sort by" seems to show old values from a previous selection

Thank you @inodb sir, just few more doubts
i) This means that when the number of samples is selected from the dropdown, the data should be sorted by the percentage value.
ii) Sorting will be shown only for the stacked bar chart; I have fixed that.

inodb · 2024-08-21T14:09:43Z

i) This means that when the number of samples is selected from the dropdown, the data should be sorted by the percentage value.

@SURAJ-SHARMA27 correct

ii) Sorting will be shown only for the stacked bar chart; I have fixed that.

Thanks so much for fixing!

SURAJ-SHARMA27 · 2024-08-21T16:57:29Z

i) This means that when the number of samples is selected from the dropdown, the data should be sorted by the percentage value.

@SURAJ-SHARMA27 correct

ii) Sorting will be shown only for the stacked bar chart; I have fixed that.

Thanks so much for fixing!

okay sir, I think I did it correctly. You can review it (pushed already). The bars will be sorted according to the percentages of the selected minor category. If the number of samples is selected from the dropdown, the bars are sorted by the total absolute count for each stack. For alphabetical sorting, the bars will be sorted as usual.

screen-capture.33.webm

sorting-functionality-added@stacked

98a4717

inodb requested review from gblaih and alisman August 7, 2024 13:34

inodb added the gsoc label Aug 7, 2024

inodb changed the title ~~sorting-functionality-added@stackedBarChart at plot tab~~ Add Stacked Bar Chart and sorting functionality Aug 7, 2024

inodb added the feature label Aug 7, 2024

inodb changed the title ~~Add Stacked Bar Chart and sorting functionality~~ Add Stacked Bar Chart and sorting to Plots Tab Aug 7, 2024

SURAJ-SHARMA27 added 2 commits August 7, 2024 21:16

x-axis-label-syncup

f3f09c2

logging-removed

2cf7b3f

sort-by-#sample_&_aphabetically-added

eb6d505

gblaih reviewed Aug 8, 2024

View reviewed changes

made-seperate-export-function

659c4ad

SURAJ-SHARMA27 requested a review from gblaih August 12, 2024 13:46

rendering-condition-fixed

3b97bad

alisman reviewed Aug 13, 2024

View reviewed changes

changes-made-a/c-review

57202bf

SURAJ-SHARMA27 closed this Aug 13, 2024

SURAJ-SHARMA27 reopened this Aug 13, 2024

logs-removed

bc6e5b9

Add Stacked Bar Chart and sorting to Plots Tab #4960

Are you sure you want to change the base?

Add Stacked Bar Chart and sorting to Plots Tab #4960

Conversation

SURAJ-SHARMA27 commented Aug 6, 2024 • edited Loading

Any screenshots or GIFs?

Notify reviewers

netlify bot commented Aug 6, 2024 • edited Loading

✅ Deploy Preview for cbioportalfrontend ready!

inodb commented Aug 7, 2024

schultzn commented Aug 7, 2024

jjgao commented Aug 7, 2024 • edited Loading

SURAJ-SHARMA27 commented Aug 7, 2024

SURAJ-SHARMA27 commented Aug 7, 2024

inodb commented Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SURAJ-SHARMA27 commented Aug 9, 2024

inodb commented Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inodb commented Aug 13, 2024 • edited Loading

SURAJ-SHARMA27 commented Aug 13, 2024

SURAJ-SHARMA27 commented Aug 13, 2024

inodb commented Aug 21, 2024 • edited Loading

SURAJ-SHARMA27 commented Aug 21, 2024

SURAJ-SHARMA27 commented Aug 6, 2024 •

edited

Loading

netlify bot commented Aug 6, 2024 •

edited

Loading

jjgao commented Aug 7, 2024 •

edited

Loading

inodb commented Aug 8, 2024 •

edited

Loading

inodb commented Aug 13, 2024 •

edited

Loading

inodb commented Aug 13, 2024 •

edited

Loading

inodb commented Aug 21, 2024 •

edited

Loading