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.

Sign up for GitHub

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

Jump to bottom

Mutation data counts #10395

Closed
wants to merge 17 commits into from
Closed

Mutation data counts #10395

wants to merge 17 commits into from

Conversation

7xuanlu
Copy link
Contributor

@7xuanlu 7xuanlu commented Sep 21, 2023 •
edited
Loading

PR for cBioPortal/icebox#568

  1. Do mutation data counts by gene specific

Option 1: based on whether the sample is mutated or not
Mutated (the samples with one or more mutations in the gene)
Wild Type (the samples without mutations in the gene)
NA (the samples that were not profiled)

Option 2: based on mutation type
Missense Mutation, Frame Shift Del, Frame Shift Ins, etc...

  1. Implement new MutationDataFilters to filter by above groups

kalletlak
kalletlak reviewed Sep 22, 2023
View reviewed changes
web/src/main/java/org/cbioportal/web/util/StudyViewFilterApplier.java Outdated
List<MutationDataFilter> mutationDataFilters = studyViewFilter.getMutationDataFilters();

if (!CollectionUtils.isEmpty(mutationDataFilters)) {
mutationDataFilters.forEach(mutationDataFilter -> {
Copy link
Member

Choose a reason for hiding this comment

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

You could use stream groupingBy lambda function

web/src/main/java/org/cbioportal/web/util/StudyViewFilterApplier.java Outdated
});
}

if (!CollectionUtils.isEmpty(mutationOptionDataFilters)) {
Copy link
Member

Choose a reason for hiding this comment

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

same logic at line 317. Why not send complete mutationDataFilters and do the grouping logic there?

web/src/main/java/org/cbioportal/web/util/StudyViewFilterApplier.java Outdated
};

FourParameterFunction<List<String>, List<String>, List<String>, String, List<ClinicalData>> fetchMutatedData = (
mappedProfileIds, mappedSampleIds, stableIds, attributeId) -> {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is same as what is at line 789

web/src/main/java/org/cbioportal/web/util/StudyViewFilterApplier.java Outdated
@@ -932,7 +936,79 @@ private <S extends DataFilter> List<ClinicalData> fetchDataAndTransformToClinica
List<String> sampleIds = new ArrayList<>();
studyViewFilterUtil.extractStudyAndSampleIds(sampleIdentifiers, studyIds, sampleIds);

if (dataFilters.get(0) instanceof GenomicDataFilter) {
if (dataFilters.get(0) instanceof MutationDataFilter) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to refactor/optimize logic in the block?

web/src/main/java/org/cbioportal/web/parameter/MutationDataFilter.java Outdated Show resolved Hide resolved
service/src/main/java/org/cbioportal/service/util/MolecularProfileUtil.java Outdated
});
}
// add case identifiers for all molecular profiles
int finalI = i;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this logic updated?

model/src/main/java/org/cbioportal/model/MutationGeneFilter.java Outdated Show resolved Hide resolved
@kalletlak kalletlak marked this pull request as ready for review September 28, 2023 14:36
@JREastonMarks
Copy link
Contributor

Overall this looks like the PR would work, my big question is could any of these lambda function be more efficiently done with a new SQL query? It seems like we are doing a lot of work with the results.

@7xuanlu
Copy link
Contributor Author

7xuanlu commented Oct 6, 2023

Overall this looks like the PR would work, my big question is could any of these lambda function be more efficiently done with a new SQL query? It seems like we are doing a lot of work with the results.

@JREastonMarks I've pushed some updates in https://github.com/cBioPortal/cbioportal/pull/10395/files#diff-afa1b11beff812038dd42375fe08a4d1990f7c5630d58e2d12a853dcb67b5d36. The refactor changes include moving java logic back to sql mapper, simplifying workloads in StudyViewController and StudyViewServiceImpl. Feel free to check it out. Let me know if anything missing! Thanks again for the suggestion.

@sonarcloud
Copy link

sonarcloud bot commented Oct 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 16 Code Smells

0.0% 0.0% Coverage
1.8% 1.8% Duplication

warning The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Dec 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 23 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.21) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@7xuanlu 7xuanlu mentioned this pull request Dec 8, 2023
Merged
@JREastonMarks
Copy link
Contributor

@h164654156465 Just an FYI that we have repackaged our code base. You should be able to merge this branch into your code base without any difficulty.

Mostly this involved moving code around so:
model/src/main/java/org/cbioportal/model/GenomicDataCount.java
is now located here:
src/main/java/org/cbioportal/model/GenomicDataCount.java

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jan 16, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

48 New issues
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@7xuanlu 7xuanlu self-assigned this Jan 19, 2024
@7xuanlu 7xuanlu added the stale label Jan 22, 2024
@JREastonMarks
Copy link
Contributor

Hi @7xuanlu ,
Can you update this with the new code in the master branch (6.0.0)? Afterwards I will be glad to review it again.

Thanks

@stale stale bot removed the stale label Jan 23, 2024
@7xuanlu
Copy link
Contributor Author

7xuanlu commented Jan 24, 2024

new code in the master branch (6.0.0)? Afterwards I will be glad to review it again.

Thanks

Hi @JREastonMarks , I've migrated this PR over to #10559. Let's work from there, thanks!

@7xuanlu 7xuanlu closed this Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JREastonMarks JREastonMarks Awaiting requested review from JREastonMarks

@kalletlak kalletlak Awaiting requested review from kalletlak

Assignees

@7xuanlu 7xuanlu

Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New study page API from mutation data counts
4 participants
@7xuanlu @JREastonMarks @kalletlak @qlu-cls