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

AG-1346 GCT bug fix for zero p-values showing wrong circle size #1282

Conversation

sagely1
Copy link
Contributor

@sagely1 sagely1 commented Feb 12, 2024

No description provided.

@sagely1 sagely1 requested a review from hallieswan February 12, 2024 20:53

if (tissue?.adj_p_val) {
size = this.getCircleSize(tissue.adj_p_val);
}

if (size === 0) {
Copy link
Contributor Author

@sagely1 sagely1 Feb 12, 2024

Choose a reason for hiding this comment

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

if circle size is zero, it should have the max size of circle as the p-value is inversely proportional to the size of the circle.

@sagely1 sagely1 requested a review from hallieswan February 19, 2024 22:55
Copy link
Contributor

@hallieswan hallieswan left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup! The refactored methods are a lot clearer. Just one small question!

Comment on lines 1022 to 1032
tissue ? tissue.logfc : '',
tissue ? tissue.ci_r : '',
tissue ? tissue.ci_l : '',
tissue ? tissue.adj_p_val : '',
tissue?.logfc || '',
tissue?.ci_r || '',
tissue?.ci_l || '',
tissue?.adj_p_val || '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this change intentional? It looks like this may reintroduce the bug fixed in AG-1347

Copy link
Contributor Author

@sagely1 sagely1 Feb 27, 2024

Choose a reason for hiding this comment

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

Good catch! Not sure why this change didn't merge from AG-1347 but I committed it.

@sagely1 sagely1 requested a review from hallieswan February 27, 2024 19:31
@sagely1 sagely1 merged commit 39c38bd into Sage-Bionetworks:develop Feb 27, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants