-
Notifications
You must be signed in to change notification settings - Fork 487
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
Demo rfc80 poc mutation data count specific gene parameter #10975
Demo rfc80 poc mutation data count specific gene parameter #10975
Conversation
@@ -27,7 +27,7 @@ public interface StudyViewRepository { | |||
|
|||
List<ClinicalData> getPatientClinicalData(StudyViewFilterContext studyViewFilterContext, List<String> attributeIds); | |||
|
|||
List<AlterationCountByGene> getMutatedGenes(StudyViewFilterContext studyViewFilterContext); | |||
List<AlterationCountByGene> getMutatedGenes(StudyViewFilterContext studyViewFilterContext, String specificHugoGeneSymbol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haynescd @fuzhaoyuan is there a way to indicate that this is optional rather than forcing people to pass null? I think that's called overloading at method, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think maybe just "hugoGeneSymbol" is enough. specificHugGeneSymbol seems redundant. There should be a comment explaining that we pass in a genesymbol if we only want counts for that gene. i also wonder whether we should just make it an array since it seems inevitable that we'll want to target multiple genes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would honestly create a new method called getMutatedGene that would reuse the sql in the mapper file for getMutatedGenes...
Kinda doesn't make sense to have getMutatedGenes that returns one gene
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'll look into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe two methods (they could be overloaded )
getMutatedGenes
getMutatedGenes(StudyViewFilter, String... geneToQuery)
@@ -27,7 +27,7 @@ public interface StudyViewRepository { | |||
|
|||
List<ClinicalData> getPatientClinicalData(StudyViewFilterContext studyViewFilterContext, List<String> attributeIds); | |||
|
|||
List<AlterationCountByGene> getMutatedGenes(StudyViewFilterContext studyViewFilterContext); | |||
List<AlterationCountByGene> getMutatedGenes(StudyViewFilterContext studyViewFilterContext, String specificHugoGeneSymbol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would honestly create a new method called getMutatedGene that would reuse the sql in the mapper file for getMutatedGenes...
Kinda doesn't make sense to have getMutatedGenes that returns one gene
@@ -42,11 +42,11 @@ public interface StudyViewRepository { | |||
|
|||
List<CaseListDataCount> getCaseListDataCountsPerStudy(StudyViewFilterContext studyViewFilterContext); | |||
|
|||
Map<String, Integer> getTotalProfiledCounts(StudyViewFilterContext studyViewFilterContext, String alterationType); | |||
Map<String, Integer> getTotalProfiledCounts(StudyViewFilterContext studyViewFilterContext, String alterationType, String specificHugoGeneSymbol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. We are overloading something that returns multiples to return a single
@@ -27,7 +27,7 @@ public interface StudyViewRepository { | |||
|
|||
List<ClinicalData> getPatientClinicalData(StudyViewFilterContext studyViewFilterContext, List<String> attributeIds); | |||
|
|||
List<AlterationCountByGene> getMutatedGenes(StudyViewFilterContext studyViewFilterContext); | |||
List<AlterationCountByGene> getMutatedGenes(StudyViewFilterContext studyViewFilterContext, String specificHugoGeneSymbol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe two methods (they could be overloaded )
getMutatedGenes
getMutatedGenes(StudyViewFilter, String... geneToQuery)
|
||
|
||
public interface StudyViewMapper { | ||
List<Sample> getFilteredSamples(@Param("studyViewFilterHelper") StudyViewFilterHelper studyViewFilterHelper); | ||
|
||
List<GenomicDataCount> getMolecularProfileSampleCounts(@Param("studyViewFilterHelper") StudyViewFilterHelper studyViewFilterHelper); | ||
|
||
List<AlterationCountByGene> getMutatedGenes(StudyViewFilterHelper studyViewFilterHelper, AlterationFilterHelper alterationFilterHelper); | ||
List<AlterationCountByGene> getMutatedGenes(StudyViewFilterHelper studyViewFilterHelper, AlterationFilterHelper alterationFilterHelper, String specificHugoGeneSymbol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you just overload the Repository layer.. you could just keep this the same except pass a list or null then you could just reuse the sql as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I got it. Thanks!
final int profiledCountWithoutGenePanelData = studyViewRepository.getTotalProfiledCountsByAlterationType(studyViewFilterContext, alterationType.toString()); | ||
var profiledCountsMap = studyViewRepository.getTotalProfiledCounts(studyViewFilterContext, alterationType.toString()); | ||
final var matchingGenePanelIdsMap = studyViewRepository.getMatchingGenePanelIds(studyViewFilterContext, alterationType.toString()); | ||
var profiledCountsMap = studyViewRepository.getTotalProfiledCounts(studyViewFilterContext, alterationType.toString(), specificHugoGeneSymbol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haynescd Do you think I should change it to list or overload? Same applies to the one below.
a73857b
to
72a5175
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use vararg to make it easier
List<GenomicDataCountItem> getMutationCountsByType(StudyViewFilterHelper studyViewFilterHelper, List<GenomicDataFilter> genomicDataFilters); | ||
|
||
AlterationCountByGene getMutatedGene(StudyViewFilterHelper studyViewFilterHelper, AlterationFilterHelper alterationFilterHelper, String specificHugoGeneSymbol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing this? I don't see it defined in the xml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry I must forgot to remove it
|
||
|
||
public interface StudyViewMapper { | ||
List<Sample> getFilteredSamples(@Param("studyViewFilterHelper") StudyViewFilterHelper studyViewFilterHelper); | ||
|
||
List<GenomicDataCount> getMolecularProfileSampleCounts(@Param("studyViewFilterHelper") StudyViewFilterHelper studyViewFilterHelper); | ||
|
||
List<AlterationCountByGene> getMutatedGenes(StudyViewFilterHelper studyViewFilterHelper, AlterationFilterHelper alterationFilterHelper); | ||
List<AlterationCountByGene> getMutatedGenes(StudyViewFilterHelper studyViewFilterHelper, AlterationFilterHelper alterationFilterHelper, List<String> hugoGeneSymbols); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a better variable name is genesToQuery?
@@ -29,6 +29,8 @@ public interface StudyViewRepository { | |||
|
|||
List<AlterationCountByGene> getMutatedGenes(StudyViewFilterContext studyViewFilterContext); | |||
|
|||
List<AlterationCountByGene> getMutatedGenes(StudyViewFilterContext studyViewFilterContext, List<String> hugoGeneSymbols); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List<AlterationCountByGene> getMutatedGenes(StudyViewFilterContext studyViewFilterContext, List<String> hugoGeneSymbols); | |
Map<String, Integer> getTotalProfiledCounts(StudyViewFilterContext studyViewFilterContext, String alterationType, String... genesToQuery); |
This is what I want. So when calling the function you can do the following...
studyViewRepository.getTotalProfiledCounts(studyViewFilter, alterationType, "TP53", "GENEA");
//or
studyViewRepository.getTotalProfiledCountsByAlterationType(studyViewFilterContext, alterationType.toString());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow that's cool
@@ -43,11 +45,15 @@ public interface StudyViewRepository { | |||
List<CaseListDataCount> getCaseListDataCountsPerStudy(StudyViewFilterContext studyViewFilterContext); | |||
|
|||
Map<String, Integer> getTotalProfiledCounts(StudyViewFilterContext studyViewFilterContext, String alterationType); | |||
|
|||
Map<String, Integer> getTotalProfiledCounts(StudyViewFilterContext studyViewFilterContext, String alterationType, List<String> hugoGeneSymbols); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the variable name to genesToQuery
Quality Gate passedIssues Measures |
Fix cBioPortal/rfc80-team#38 and cBioPortal/rfc80-team#43
Describe changes proposed in this pull request: