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-1344 scaling bug fix and refactor of gene evidence box plots #1281

Merged
merged 2 commits into from
Feb 27, 2024

Conversation

sagely1
Copy link
Contributor

@sagely1 sagely1 commented Feb 5, 2024

No description provided.

@@ -34,6 +34,7 @@ export class BoxPlotComponent extends BaseChartComponent {
@Input() yAxisLabel = 'LOG 2 FOLD CHANGE';
@Input() yAxisMin: number | undefined;
@Input() yAxisMax: number | undefined;
@Input() yAxisPadding = 0.2;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now the chart has the ability to have a padding specified

@sagely1 sagely1 requested a review from hallieswan February 5, 2024 19:03
@sagely1 sagely1 force-pushed the AG-1344-proteomics-plot branch from 7e0740a to 0c19150 Compare February 12, 2024 12:00
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.

These changes look good!

While looking at the examples in the ticket, I noticed that the TMT plot for MDK still isn't shown, even though there is data available. I think this may be related to this code , where selectedUniProtId is determined using the uniprotids from both proteomics_LFQ and proteomics_TMT. For MDK, proteomics_LFQ has E9PPJ5 and P21741 as uniprotids, but proteomics_TMT only has P21741 as uniprotid. E9PPJ5 is set as the selectedUniProtId, but is only found in the LFQ data, so no data is displayed for the TMT plot. Would you mind taking a look? Up to you whether to make the fix here or open a new ticket to track!

@sagely1
Copy link
Contributor Author

sagely1 commented Feb 23, 2024

These changes look good!

While looking at the examples in the ticket, I noticed that the TMT plot for MDK still isn't shown, even though there is data available. I think this may be related to this code , where selectedUniProtId is determined using the uniprotids from both proteomics_LFQ and proteomics_TMT. For MDK, proteomics_LFQ has E9PPJ5 and P21741 as uniprotids, but proteomics_TMT only has P21741 as uniprotid. E9PPJ5 is set as the selectedUniProtId, but is only found in the LFQ data, so no data is displayed for the TMT plot. Would you mind taking a look? Up to you whether to make the fix here or open a new ticket to track!

Good observation. The nature of the data is that there are times that data for LFQ or TMT may not exist. In that case, we can allow the plots to show as 'no data is currently available'.

@sagely1 sagely1 closed this Feb 23, 2024
@sagely1 sagely1 reopened this Feb 23, 2024
@sagely1 sagely1 requested a review from hallieswan February 23, 2024 23:34
@@ -54,10 +54,6 @@ export class GeneEvidenceProteomicsComponent {
init() {
this.reset();

if (!this._gene?.proteomics_LFQ && this._gene?.proteomics_TMT) {
Copy link
Contributor Author

@sagely1 sagely1 Feb 23, 2024

Choose a reason for hiding this comment

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

Original code is confusing and these props are always defined.

@hallieswan
Copy link
Contributor

Good observation. The nature of the data is that there are times that data for LFQ or TMT may not exist. In that case, we can allow the plots to show as 'no data is currently available'.

Oh I see! I didn't notice the dropdown for selecting the uniprotid of interest, which determines which data to show. Thanks for helping me understand!

@sagely1 sagely1 merged commit 8c34e32 into Sage-Bionetworks:develop Feb 27, 2024
4 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