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

Binning of custom data #10039

Merged
merged 3 commits into from
Mar 17, 2023

Conversation

BasLee
Copy link

@BasLee BasLee commented Feb 20, 2023

Allow users to bin and filter their own custom (numerical and categorical) datasets using a new endpoint.

These changes will be used in a front end PR.

Changes

  • Add new endpoint /custom-data-bin-counts/fetch to StudyViewController to bin custom datasets
  • Isolate retrieval of custom data from session service in new CustomDataServiceImpl, and share functionality at relevant places
  • Filter by both categorical and numerical custom data using CustomDataFilterApplier
  • Refactor ClinicalDataBinUtil to bin both clinical and custom data.

Tests

  • Filtering is tested in StudyViewFilterApplierTest
  • Binning is tested in ClinicalDataBinUtilTest
  • Use applicationContext-web-test.xml in integration tests to fix missing CustomDataServiceImpl (and related) beans

Copy link
Member

@onursumer onursumer left a comment

Choose a reason for hiding this comment

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

Looks good to me in general. I can't see any major issues with the PR. Thanks!

One quick question. Why do we need to include the binary frontend-cbioportal-0.3.0.jar file?

@@ -0,0 +1,5093 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need a test file this big? :)

Copy link
Author

Choose a reason for hiding this comment

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

Reduced the size of the test file in dc81111

@BasLee
Copy link
Author

BasLee commented Feb 27, 2023

Looks good to me in general. I can't see any major issues with the PR. Thanks!

One quick question. Why do we need to include the binary frontend-cbioportal-0.3.0.jar file?

Jar is removed in dc81111

@inodb inodb added the api label Feb 27, 2023
@BasLee BasLee force-pushed the custom_data_binning_endpoint branch 2 times, most recently from 61685ad to 1625740 Compare March 2, 2023 09:41
Copy link
Member

@onursumer onursumer left a comment

Choose a reason for hiding this comment

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

Looks good to me

@inodb inodb requested a review from dippindots March 9, 2023 15:43
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.

@BasLee It looks good to me, thanks for performing refactoring work to make it organized. Most of my comments are about style or some code that might need clarification. I don't see anything wrong with it. Please feel free to reach out to me if there is anything not clear in my comments. Thanks!

model/src/main/java/org/cbioportal/model/ClinicalData.java Outdated Show resolved Hide resolved
uniqueSampleKeys.addAll(studyViewFilterApplier.getUniqkeyKeys(studyIds, sampleIds));
uniquePatientKeys.addAll(studyViewFilterApplier.getUniqkeyKeys(studyIdsOfPatients, patientIds));

if (clinicalAttributes != null && ! clinicalAttributes.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to leave no space between logical not and variable, maybe we can change to

Suggested change
if (clinicalAttributes != null && ! clinicalAttributes.isEmpty()) {
if (clinicalAttributes != null && !clinicalAttributes.isEmpty()) {

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 948c8b9

web/src/main/java/org/cbioportal/web/util/IdPopulator.java Outdated Show resolved Hide resolved
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.

Thanks, @BasLee, it looks good to me! I will rebase your pr to the latest master. If everything looks good, I will merge it then.

@sonarcloud
Copy link

sonarcloud bot commented Mar 15, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug B 1 Bug
Vulnerability C 1 Vulnerability
Security Hotspot A 0 Security Hotspots
Code Smell A 28 Code Smells

0.0% 0.0% Coverage
6.1% 6.1% Duplication

@dippindots
Copy link
Member

@inodb @BasLee I looked at the failing localdb tests, the failing tests are not related to this pull request, I will merge this one.

@dippindots dippindots merged commit ca13827 into cBioPortal:master Mar 17, 2023
@inodb
Copy link
Member

inodb commented Mar 17, 2023

Thanks @dippindots !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants