-
Notifications
You must be signed in to change notification settings - Fork 303
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
on plots tab add sorting for barcharts #4893
on plots tab add sorting for barcharts #4893
Conversation
✅ Deploy Preview for cbioportalfrontend ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I missed one simple condition ,now I have added it. This searching functionality will only work for stacked bar graphs.
// reverse the order of stacked or horizontal bars | ||
if ((!horizontalBars && stacked) || (horizontalBars && !stacked)) { | ||
data = _.reverse(data); | ||
} | ||
console.log(data, 'iscalled2'); | ||
data.forEach(item => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this whole logic looks similar to the one above
this.data.forEach(item => { |
would be great if we could make a helper function with all this logic to minimize duplicate code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed! can this be DRY'd?
console.log(selectedOption, 'thisisselectedoptions'); | ||
console.log(this.sortOption, 'thisSOrtts'); | ||
this.sortOption = selectedOption.value; | ||
// this.setState({ sortOption: selectedOption.value }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove these comments now
@gblaih I have made nearly all the changes you suggested. Could you please review them and merge the branch if everything looks good? |
|
||
const sortedLabels = totalSumArray.map(item => item.majorCategory); | ||
|
||
if (this.props.sortOption == 'sortByCount') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can move all the logic above this to inside this if block as we only need to calculate sortedLabels when sortOption
is sortByCount
. same for the similar function in MultipleCategoryBarPlotUtils.ts
interface TotalSumItem { | ||
majorCategory: string; | ||
sum: number; | ||
minorCategory: { name: string; count: number; percentage: number }[]; | ||
} | ||
|
||
export { TotalSumItem }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can delete this and just import and use the one you made here
export interface TotalSumItem { |
|
||
const finals = minorCategoryArrays[minorCategory]; | ||
|
||
let categoriezedCounts; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
categorizedCounts
@@ -937,6 +937,7 @@ export default class ClinicalData extends React.Component< | |||
? this.highlightedRow.qValue | |||
: null | |||
} | |||
sortOption={'sortByAlphabet'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can probably delete this and the other instances of passing sortByAlphabet
throughout the code now that we made sortOption
optional and we've taken care of the case when sortOption
is not sortByCount
@SURAJ-SHARMA27 added a couple minor comments to be addressed but looks good overall. thanks! |
@gblaih , I have made all the changes suggested. Could you review it once more? |
@@ -424,12 +435,56 @@ export default class MultipleCategoryBarPlot extends React.Component< | |||
} | |||
|
|||
@computed get labels() { | |||
if (this.props.sortOption == 'sortByCount') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SURAJ-SHARMA27 this is going to actually mutate the sorting of counts in this.data
. i.e. sort it in place. instead, we should use functional/immutable approach to return a sorted list of labels without mutating underlying data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the mutable approach and created a separate totalSumArray, which does not make any changes to this.data
a.majorCategory.localeCompare(b.majorCategory) | ||
); | ||
}); | ||
const totalSumArray: TotalSumItem[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code is quite confusing. it would be clearer if you used _.sortBy(collection, ()=>); where the return of the callback is a _.sum of the item counts within.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is that I will traverse this.data and will make one totalSumArray array which has a schema like:
{
majorCategory: countItem.majorCategory,
sum: countItem.count,
minorCategory: [
{
name: item.minorCategory,
count: countItem.count,
percentage: countItem.percentage,
},
],
}
Now, I will traverse this.data and go to each majorCategory
to update the totalSum
in which i will store the count of each object. Then, I will sort the totalSumArray
with respect to totalSum
and extract the information as per the requirements like label and majorCategory
.
Let's take an example. For one configuration, this.data contains:
{
0: { minorCategory: 'Male', counts: Array(42) },
1: { minorCategory: 'Female', counts: Array(42) }
}
I will traverse each object, go to the counts array, and create one object for like Melanoma in totalSumArray
in which I will store the information and sum of counts for each object. Then, I will sort the totalSumArray
according to sum and retrieve the labels from there.
@@ -424,12 +435,56 @@ export default class MultipleCategoryBarPlot extends React.Component< | |||
} | |||
|
|||
@computed get labels() { | |||
if (this.props.sortOption == 'sortByCount') { | |||
_.forEach(this.data, item => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm curious why you need to sort alphabetical first. you should put comments explaining each step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was sorting the elements alphabetically first so that all the elements in each object would come in the same order, allowing me to calculate the count corresponding to each element accurately. For example:
data = {
0: { minorCategory: 'Male', counts: Array(42) },
1: { minorCategory: 'Female', counts: Array(42) }
}
Let's say, in the array containing 42 elements in the Male minorCategory, the first index is X, and in the Female minorCategory, the first index is Y in counts. If I calculate sum_of_total_counts = data[0].counts[0].count + data[1].counts[1].count, the sum_of_total_counts calculated is incorrect because it adds the counts for X and Y. However, if I sort it alphabetically, the counts array for each minorCategory will be in the same order, and I can find the total count easily.
@@ -739,7 +794,8 @@ export default class MultipleCategoryBarPlot extends React.Component< | |||
this.categoryCoord, | |||
!!this.props.horizontalBars, | |||
!!this.props.stacked, | |||
!!this.props.percentage | |||
!!this.props.percentage, | |||
this.props.sortOption || 'sortByAlphabet' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should probably apply this default inside the makeBarSpec
@@ -4448,6 +4464,32 @@ export default class PlotsTab extends React.Component<IPlotsTabProps, {}> { | |||
</label> | |||
</div> | |||
)} | |||
{this.isStacked && (this.plotType.result==3) && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make the 3 into an Enum so it's human understandable?
@@ -5193,7 +5235,16 @@ export default class PlotsTab extends React.Component<IPlotsTabProps, {}> { | |||
DownloadControlOption.SHOW_ALL | |||
); | |||
} | |||
|
|||
get isStacked() { | |||
const isPercentage = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confusing structure. clearer if get rid of isPercentage local var and you just spell it out:
A === B || A === C
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SURAJ-SHARMA27 i think the code here can be improved. the biggest issue is the sorting of what should be immutable data. see comments. one good rule of thumb is to avoid forEach. The reason is that forEach forces you do do a sideeffect because it isn't "functional". It doesn't simply return a new thing (e.g. a sorted array that is a computation of underlying array). Always use more specific operators like map/reduce/sort etc. Learing the full set of Lodash functions will help you.
@SURAJ-SHARMA27 have you had a chance to look at these comments? |
Actually, my laptop's RAM was causing issues, which is why the repository was not running locally. |
This is superseded by: #4960 |
Fixes #10722
Describe changes proposed in this pull request:
I have created a dropdown field named 'Sort' which currently contains two options: 'Sort by Alphabet' and 'Sort by Count'. By default, 'Sort by Alphabet' is selected. I implemented the logic for 'Sort by Count' as follows:
i) I passed the 'sortOption' prop throughout the plot components.
ii) In 'MultipleCategoryBarPlotUtils.ts', I added a case where if the 'sortOption' is 'sortByCount', it creates an array named 'totalSumArray'. This array stores fields defined by the interface:
Then, I sorted the above array with respect to 'sum', and finally returned the individually sorted array. I implemented the same for sorted labels.
Checks
Any screenshots or GIFs?
Screencast.from.2024-04-16.15-48-09.webm