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

changes for #9943 on frontend #4576

Closed
wants to merge 1 commit into from

Conversation

Eric-Jiang1
Copy link

Fix cBioPortal/cbioportal#9943

Describe changes proposed in this pull request:

  • Fisher's exact test is giving the two-sided value
  • changed mutual exclusivity tab and comparison tab to now show "2-sided fisher's exact test" instead of 1-sided to users
  • added in comments to walk future developers through

Checks

  • Has tests or has a separate issue that describes the types of test that should be created. If no test is included it should explicitly be mentioned in the PR why there is no test.
  • The commit log is comprehensible. It follows 7 rules of great commit messages. For most PRs a single commit should suffice, in some cases multiple topical commits can be useful. During review it is ok to see tiny commits (e.g. Fix reviewer comments), but right before the code gets merged to master or rc branch, any such commits should be squashed since they are useless to the other developers. Definitely avoid merge commits, use rebase instead.
  • Is this PR adding logic based on one or more clinical attributes? If yes, please make sure validation for this attribute is also present in the data validation / data loading layers (in backend repo) and documented in File-Formats Clinical data section! (This PR is not adding logic based on one or more clinical attributes in the backend repo)

Notify reviewers

Doing this through Slack, I've been actively communicating with the team to resolve this issue.

@dippindots dippindots self-requested a review April 11, 2023 14:34
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.

Great to see you added some comments, that's very helpful. One thing is although backend changes will make comparison tabs using the two-sided Fisher Exact test, the MutualExclusivityTable.tsx is still using the one-sided Fisher Exact test which is implemented in the front-end, so we might want to change this too.
It would also be great if we can have some unit tests to verify the newly implemented method.
Please see this example on how to create unit tests for a method.

@@ -19,6 +21,7 @@ function getProbability(
return Math.exp(p);
}

// Cumulative p-value for a 2x2 contingency table using Fisher's exact test
export function getCumulativePValue(
Copy link
Member

Choose a reason for hiding this comment

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

I think in MutualExclusivityTable, we are using getCumulativePValue function in FisherExactTestCalculator.ts to calculate one-sided Fisher's exact test right now. So we might need to implement a new method for this table to calculate two-sided Fisher's exact test. You can reference the backend implementation: https://github.com/cBioPortal/cbioportal/blob/master/core/src/main/java/org/mskcc/cbio/portal/stats/FisherExact.java#L240

@gblaih gblaih self-requested a review April 17, 2023 14:39
@inodb
Copy link
Member

inodb commented Jun 8, 2023

Thanks @Eric-Jiang1 ! We will add this to #4556

@inodb inodb closed this Jun 8, 2023
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.

change 1-sided fisher exact test to two-sided
3 participants