Add mutations table and fisher exact two-sided p value to comparison lollipop #4556
Conversation
gblaih
force-pushed
the
group-comparison-lollipop-plot-mutations-table
branch
2 times, most recently
from
0b7979f
to
f9b8f3b
Compare
gblaih
force-pushed
the
group-comparison-lollipop-plot-mutations-table
branch
from
efc68e9
to
5abb62d
Compare
gblaih
force-pushed
the
group-comparison-lollipop-plot-mutations-table
branch
from
600a1de
to
f854c96
Compare
| @@ -7,6 +7,7 @@ import styles from './filterResetPanel.module.scss'; | |||
| type FilterResetPanelProps = { | |||
| resetFilters: () => void; | |||
| filterInfo?: JSX.Element | string; | |||
| shiftClickMessage?: JSX.Element | string; | |||
There was a problem hiding this comment.
I would name this prop additionalInfo or something like that. FilterResetPanel doesn't need to know what a shift+click is.
gblaih
force-pushed
the
group-comparison-lollipop-plot-mutations-table
branch
3 times, most recently
from
c19def2
to
d8cbb9d
Compare
| @@ -34,14 +35,18 @@ export class FilterResetPanel extends React.Component< | |||
| data-test="filter-reset-panel" | |||
| > | |||
| <span style={{ verticalAlign: 'middle' }}> | |||
| {this.props.filterInfo} | |||
| <span style={{ fontWeight: 'bold' }}> | |||
There was a problem hiding this comment.
use <strong> tag instead of inline style
packages/react-mutation-mapper/src/store/DefaultMutationMapperFilterApplier.ts
Show resolved
Hide resolved
| groupToProfiledPatientCounts, | ||
| }: IFisherExactTwoSidedTestLabelProps) => { | ||
| const mutationCountForActiveGeneGroupA = countUniqueMutations( | ||
| _.intersection( |
There was a problem hiding this comment.
we've had performance problems with intersect before. look up the package "fast-array-intersect" which we use elsehwere in our project and i think will be must faster.
| ) | ||
| ); | ||
|
|
||
| const getFisherTestLabel = () => { |
There was a problem hiding this comment.
doesn't really make sense to declare function here. if you want encapsulate and test this logic then put function outside and pass it the variables it needs. otherwise, just assign to local variable.
| }; | ||
|
|
||
| return ( | ||
| <div style={{ fontWeight: 'bold' }}> |
There was a problem hiding this comment.
wrap in <strong> inside of div
| } | ||
|
|
||
| @observer | ||
| export default class GroupComparisonMutationMapper extends MutationMapper< | ||
| IGroupComparisonMutationMapperProps | ||
| > { | ||
| @observable.ref _enrichedGroups: string[] = this.props.groups.map( |
There was a problem hiding this comment.
i would do this assignment in the constructor. makes it more clear that we are starting with something that comes from a prop but that henceforth this state will be managed internally and can be out of sync with the prop. is this the intention?
| @observable.ref _enrichedGroups: string[] = this.props.groups.map( | ||
| group => group.nameWithOrdinal | ||
| ); | ||
| @observable significanceFilter: boolean = false; |
There was a problem hiding this comment.
not a good name for a boolean. hasSignificanceFilter? singnificanceFilterEnabled?
| @@ -46,8 +76,119 @@ export default class GroupComparisonMutationMapper extends MutationMapper< | |||
| return null; | |||
| } | |||
|
|
|||
| protected formatPaginationStatusText = (text: string) => { | |||
There was a problem hiding this comment.
this should really be a free function (not a method of class) which accepts text and tableData as arguments. would be much easier to test.
| ? 'Mutation' | ||
| : 'Mutations'; | ||
|
|
||
| return text.includes('Mutations') |
There was a problem hiding this comment.
interesting (and yucky!) we should explain this with a comment if it must be.
There was a problem hiding this comment.
yeah, we really need a comment on this whole function to explain what it does
|
|
||
| @computed get rowDataByProteinChange() { | ||
| let dataStore = this.props.store.dataStore as MutationMapperDataStore; | ||
| let mutationsByProteinChange = _.values( |
There was a problem hiding this comment.
you should avoid nesting lodash. use chaining instead.
also, this seems strange because, unless i'm mistaken, will result in array of arrays? where mutationsByPorteinChange suggests a map of proteinChange to mutations.
| return _.pickBy(this.rowDataByProteinChange, v => { | ||
| return this.significanceFilter | ||
| ? this._enrichedGroups.includes(v.enrichedGroup) && | ||
| v.qValue < 0.05 |
There was a problem hiding this comment.
0.05 should be a constant defined somewhere and we should explain in comment it's meaning. having a constant name will help to explain it.
|
|
||
| @autobind | ||
| protected mutationsGroupedByProteinChangeForGroup(groupIndex: number) { | ||
| let allGroupedData = this.store.dataStore.allData; |
There was a problem hiding this comment.
try to avoid using let in general. here, you could just do
const allGroupedData = groupDataByGroupFilters with third arg being this.store.data.store.allData, no?
| .uniq() | ||
| .value(); | ||
|
|
||
| const filters = proteinChanges.map(value => ({ |
There was a problem hiding this comment.
just combine this with chain above and use comments to explain/label the steps.
| _.forIn( | ||
| this.mutationsGroupedByProteinChangeForGroup(groupIndex), | ||
| (v, k) => { | ||
| const uniqueMutations = _.uniqBy(v.data, d => d[0].patientId); |
There was a problem hiding this comment.
put in a comment here why it's uniq by patient
| type="checkbox" | ||
| checked={this.significanceFilter} | ||
| onClick={this.toggleSignificanceFilter} | ||
| data-test="SwapAxes" |
There was a problem hiding this comment.
give it a test handle that's a little more specific to feature.
gblaih
force-pushed
the
group-comparison-lollipop-plot-mutations-table
branch
4 times, most recently
from
0cf182a
to
ca3ad0d
Compare
There was a problem hiding this comment.
Thanks @gblaih ! This looks good to me
As it also partially addresses: cBioPortal/cbioportal#9943. Could we do a follow up PR that updates FAQ etc?
… dropdown, shift click message. specified fisher test use in text
…abel displays numbers used, changes to layout
…parison filter alert and table
…d table header styling
gblaih
force-pushed
the
group-comparison-lollipop-plot-mutations-table
branch
from
ca3ad0d
to
42431de
Compare
gblaih
force-pushed
the
group-comparison-lollipop-plot-mutations-table
branch
from
42431de
to
24bf9a8
Compare
Implements cBioPortal/cbioportal#10012