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 Stacked Bar Chart and sorting to Plots Tab #4960

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

SURAJ-SHARMA27
Copy link
Contributor

@SURAJ-SHARMA27 SURAJ-SHARMA27 commented Aug 6, 2024

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

Copy link

netlify bot commented Aug 6, 2024

Deploy Preview for cbioportalfrontend ready!

Name Link
🔨 Latest commit 7db2e6d
🔍 Latest deploy log https://app.netlify.com/sites/cbioportalfrontend/deploys/6790284c63371a000887f4fc
😎 Deploy Preview https://deploy-preview-4960.preview.cbioportal.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.

@inodb inodb requested review from gblaih and alisman August 7, 2024 13:34
@inodb inodb added the gsoc label Aug 7, 2024
@inodb inodb changed the title sorting-functionality-added@stackedBarChart at plot tab Add Stacked Bar Chart and sorting functionality Aug 7, 2024
@inodb inodb added the feature label Aug 7, 2024
@inodb inodb changed the title Add Stacked Bar Chart and sorting functionality Add Stacked Bar Chart and sorting to Plots Tab Aug 7, 2024
@inodb
Copy link
Member

inodb commented Aug 7, 2024

@SURAJ-SHARMA27 this looks awesome - thank you!

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

image

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

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

@schultzn
Copy link

schultzn commented Aug 7, 2024

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
Copy link
Member

jjgao commented Aug 7, 2024

Good work @SURAJ-SHARMA27 !

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

image

  • 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
Copy link
Contributor Author

Good work @SURAJ-SHARMA27 !

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

image

  • 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
Copy link
Contributor Author

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
Copy link
Member

inodb commented Aug 8, 2024

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:
    image

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

Choose a reason for hiding this comment

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

sortByDropDownOptions

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

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

Choose a reason for hiding this comment

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

===

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

Choose a reason for hiding this comment

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

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

Comment on lines 5436 to 5441
this?.SortByDropDownOptions
}
updateDropDownOptions={
this?.updateDropDownOptions
}
sortByOption={this?.sortByOption}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Comment on lines 433 to 436
if (this.props.sortByOption == 'SortByTotalSum') {
const majorCategoryCounts: any = {};

this.data.forEach(item => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

(a, b) => majorCategoryCounts[b] - majorCategoryCounts[a]
);

const reorderCounts = (counts: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@SURAJ-SHARMA27
Copy link
Contributor Author

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

@SURAJ-SHARMA27 SURAJ-SHARMA27 requested a review from gblaih August 12, 2024 13:46
@inodb
Copy link
Member

inodb commented Aug 13, 2024

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

image

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

private setInitialSelectedOption = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

avoid 'any'. lets type this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have defined an enumeration and used it

const sortedMajorCategories = getSortedMajorCategories(data, sortByOption);

if (sortByOption === 'SortByTotalSum' || sortedMajorCategories.length > 0) {
const reorderCounts = (counts: any) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

avoid 'any'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@inodb
Copy link
Member

inodb commented Aug 21, 2024

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
Copy link
Contributor Author

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

@inodb inodb force-pushed the sorting-functionality@stackedBarChart branch from bc6e5b9 to bbafcbf Compare October 24, 2024 14:36
@alisman alisman force-pushed the sorting-functionality@stackedBarChart branch from bbafcbf to 903134d Compare December 20, 2024 17:14
@inodb inodb mentioned this pull request Dec 30, 2024
3 tasks
@alisman alisman force-pushed the sorting-functionality@stackedBarChart branch 3 times, most recently from f7f261a to 6abbaf5 Compare January 10, 2025 16:27
@alisman
Copy link
Collaborator

alisman commented Jan 10, 2025

Hi @SURAJ-SHARMA27,
There is a regression error caused by this PR. I realize GSOC is long over. Do you have a few minutes to look at the issue?

If you visit this link with the PR active, you'll see the following problem:

image

If you are busy with school or work, we understand and will find someone else to look into it.

@dippindots
Copy link
Member

Related ticket: cBioPortal/cbioportal#10722

@alisman
Copy link
Collaborator

alisman commented Jan 14, 2025

@SURAJ-SHARMA27 just FYI, we have a dev @gblaih who is taking care of the last fixes.

@gblaih gblaih force-pushed the sorting-functionality@stackedBarChart branch from 302c00b to a9f5d3d Compare January 21, 2025 22:02
@dippindots dippindots assigned dippindots and gblaih and unassigned dippindots Jan 22, 2025
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.

7 participants