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

Bugfix/make the inferred cell type options dynamic shown or disabled in marker gene heatmap dropdown menu #482

Merged
merged 35 commits into from
Dec 13, 2024
Merged

Bugfix/make the inferred cell type options dynamic shown or disabled in marker gene heatmap dropdown menu #482

merged 35 commits into from
Dec 13, 2024

Conversation

upendrakumbham
Copy link
Contributor

@upendrakumbham upendrakumbham commented Oct 24, 2024 •
edited
Loading

Issue reference - Issue

@upendrakumbham
Fetch marker gene id with experiment accession from the DB
8e7177a 
Signed-off-by: upendrakumbham <[email protected]>
@upendrakumbham
Merge branch 'develop' into make-the-inferred-cell-type-options-dynam…
9b74beb 
…ic-shown-or-disabled-in-marker-gene-heatmap-dropdown-menu
@upendrakumbham
Fix broken tests
4ab3665 
Signed-off-by: upendrakumbham <[email protected]>
@upendrakumbham
Update SQL query
2e3cb5d 
Signed-off-by: upendrakumbham <[email protected]>
@ke4 ke4 added the bug Something isn't working label Nov 5, 2024
@upendrakumbham upendrakumbham marked this pull request as ready for review November 13, 2024 11:15
@upendrakumbham
Fix broken tests
5c1fccc 
Signed-off-by: upendrakumbham <[email protected]>
@upendrakumbham
Add Service layer tests
918127c 
Signed-off-by: upendrakumbham <[email protected]>
@upendrakumbham
Add service integration tests
9f9fa67 
Signed-off-by: upendrakumbham <[email protected]>
@upendrakumbham
Improve getMarkerGeneMetadata method
5ff7093 
Signed-off-by: upendrakumbham <[email protected]>
ke4
ke4 requested changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@ke4 ke4 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 your work @upendrakumbham .
I added a couple of comments/suggestions.
Could you please check them?
Thanks

app/src/test/java/uk/ac/ebi/atlas/experimentpage/tabs/ExperimentPageContentServiceTest.java Outdated Show resolved Hide resolved
app/src/main/java/uk/ac/ebi/atlas/experimentpage/tabs/ExperimentPageContentService.java Outdated Show resolved Hide resolved
app/src/main/java/uk/ac/ebi/atlas/experimentpage/tabs/ExperimentPageContentService.java Show resolved Hide resolved
app/src/main/java/uk/ac/ebi/atlas/experimentpage/tabs/ExperimentPageContentService.java Outdated Show resolved Hide resolved
app/src/main/java/uk/ac/ebi/atlas/experimentpage/tabs/ExperimentPageContentService.java Outdated Show resolved Hide resolved
app/src/main/java/uk/ac/ebi/atlas/experimentpage/tabs/ExperimentPageContentService.java Outdated Show resolved Hide resolved
app/src/main/java/uk/ac/ebi/atlas/experimentpage/tabs/ExperimentPageContentService.java Outdated Show resolved Hide resolved
app/src/test/java/uk/ac/ebi/atlas/experimentpage/markergenes/MarkerGeneServiceIT.java Outdated Show resolved Hide resolved
app/src/test/java/uk/ac/ebi/atlas/experimentpage/markergenes/MarkerGeneServiceTest.java Outdated Show resolved Hide resolved
app/src/test/java/uk/ac/ebi/atlas/experimentpage/markergenes/MarkerGenesDaoIT.java Outdated Show resolved Hide resolved
@upendrakumbham
Copy link
Contributor Author

upendrakumbham commented Nov 25, 2024 •
edited
Loading

Thanks for the review @ke4. I'm working on the code refactoring.

@upendrakumbham upendrakumbham mentioned this pull request Nov 13, 2024
Open
upendrakumbham and others added 8 commits November 28, 2024 12:26
@upendrakumbham
Fix review comments - remove regx, remove unnecessary code and code i…
f3c04f2 
…mprovement

Signed-off-by: upendrakumbham <[email protected]>
@upendrakumbham @ke4
Update test method name
11c992b 
Co-authored-by: Károly Erdős <[email protected]>
@upendrakumbham @ke4
Improve code
061dd39 
Co-authored-by: Károly Erdős <[email protected]>
@upendrakumbham
Merge branch 'develop' into bugfix/make-the-inferred-cell-type-option…
2bcfce7 
…s-dynamic-shown-or-disabled-in-marker-gene-heatmap-dropdown-menu
@upendrakumbham @ke4
Improve code
7bc29be 
Co-authored-by: Károly Erdős <[email protected]>
@upendrakumbham @ke4
Update test
cffd5c9 
Co-authored-by: Károly Erdős <[email protected]>
@upendrakumbham
Code cleanup and improvements
cade7ec 
Signed-off-by: upendrakumbham <[email protected]>
@upendrakumbham
Remove unused methods
1ad1bcd 
Signed-off-by: upendrakumbham <[email protected]>
@upendrakumbham
Replace Sys err with logger
a4575ab 
Signed-off-by: upendrakumbham <[email protected]>
@upendrakumbham
Refactor variables names
6c3a201 
Signed-off-by: upendrakumbham <[email protected]>
ke4
ke4 requested changes Dec 5, 2024
View reviewed changes
Copy link
Contributor

@ke4 ke4 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 your work @upendrakumbham .
I added some tiny suggestions, other than that all is fine.
Thanks

app/src/main/java/uk/ac/ebi/atlas/experimentpage/tabs/ExperimentPageContentService.java Outdated Show resolved Hide resolved
app/src/main/java/uk/ac/ebi/atlas/experimentpage/tabs/ExperimentPageContentService.java Show resolved Hide resolved
app/src/main/java/uk/ac/ebi/atlas/experimentpage/tabs/ExperimentPageContentService.java Outdated Show resolved Hide resolved
app/src/test/java/uk/ac/ebi/atlas/experimentpage/markergenes/MarkerGeneServiceIT.java Show resolved Hide resolved
app/src/test/java/uk/ac/ebi/atlas/experimentpage/markergenes/MarkerGenesDaoIT.java Outdated Show resolved Hide resolved
app/src/test/java/uk/ac/ebi/atlas/experimentpage/tabs/ExperimentPageContentServiceTest.java Outdated Show resolved Hide resolved
app/src/test/java/uk/ac/ebi/atlas/experimentpage/tabs/ExperimentPageContentServiceTest.java Outdated Show resolved Hide resolved
upendrakumbham and others added 6 commits December 5, 2024 14:09
@upendrakumbham
Fix broken test
da7062e 
Signed-off-by: upendrakumbham <[email protected]>
@upendrakumbham @ke4
Improve test method name
d353670 
Co-authored-by: Károly Erdős <[email protected]>
@upendrakumbham @ke4
Rename test method
d5bd597 
Co-authored-by: Károly Erdős <[email protected]>
@upendrakumbham @ke4
Update code
72f2036 
Co-authored-by: Károly Erdős <[email protected]>
@upendrakumbham @ke4
Update access modifier
6adbc57 
Co-authored-by: Károly Erdős <[email protected]>
@upendrakumbham
Update variable name
cd347f2 
Signed-off-by: upendrakumbham <[email protected]>
@upendrakumbham upendrakumbham requested a review from ke4 December 5, 2024 15:02
ke4
ke4 previously approved these changes Dec 5, 2024
View reviewed changes
Copy link
Contributor

@ke4 ke4 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You have a failing test that you wrote in this change set: getMarkerGenesCountForInferredCellTypeAuthorsLabels.
Could you fix it please?
Other than that it is all OK!
Thanks for your work!

@upendrakumbham
Fix broken test
5686849 
Signed-off-by: upendrakumbham <[email protected]>
@upendrakumbham
Copy link
Contributor Author

You have a failing test that you wrote in this change set: getMarkerGenesCountForInferredCellTypeAuthorsLabels. Could you fix it please? Other than that it is all OK! Thanks for your work!

I fixed test case, in my local test case is passing:

MarkerGeneServiceTest > Fetch marker gene profile expression from ontology label cell types PASSED

MarkerGeneServiceTest > getMarkerGenesCountZero_IfTheMarkerGenesDoesNotExistForTheAuthorsLabels() PASSED

MarkerGeneServiceTest > Returns empty profile if both ontology labels returns empty cell type values PASSED

MarkerGeneServiceTest > Fetch marker gene profile expression from authors label cell types PASSED

MarkerGeneServiceTest > getMarkerGenesCountForTheInferredCelltypeOntologyLabels() PASSED

MarkerGeneServiceTest > getMarkerGenesCountZero_IfTheMarkerGenesDoesNotExistForTheOntologyLabels() PASSED

MarkerGeneServiceTest > getMarkerGenesCountForInferredCellTypeAuthorsLabels() PASSED

But Jenkins build still failing. I have triggered the build again in Jenkins. I will wait for the result.

@upendrakumbham
Rename test methods
d1d487f 
Signed-off-by: upendrakumbham <[email protected]>
@ke4
Copy link
Contributor

ke4 commented Dec 6, 2024

But Jenkins build still failing. I have triggered the build again in Jenkins. I will wait for the result.

@upendrakumbham Do we have the same data locally comparing that we have in Jenkins?
Maybe the data is different and that's why this test failed.

@upendrakumbham
Merge branch 'develop' into bugfix/make-the-inferred-cell-type-option…
6bd29a6 
…s-dynamic-shown-or-disabled-in-marker-gene-heatmap-dropdown-menu
@upendrakumbham
Update tests
98eefdb 
Signed-off-by: upendrakumbham <[email protected]>
ke4
ke4 previously approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@ke4 ke4 left a comment

Choose a reason for hiding this comment

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

LGTM

@upendrakumbham
Merge branch 'develop' into bugfix/make-the-inferred-cell-type-option…
a210be5 
…s-dynamic-shown-or-disabled-in-marker-gene-heatmap-dropdown-menu
lingyun1010
lingyun1010 approved these changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@lingyun1010 lingyun1010 left a comment

Choose a reason for hiding this comment

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

I just approved the test fix PR from Karoly #491 (review), which may resolve the test issue. Anyway, LGTM!

@upendrakumbham
Copy link
Contributor Author

I just approved the test fix PR from Karoly #491 (review), which may resolve the test issue. Anyway, LGTM!

I got a green in the Jenkins , so merging it

@upendrakumbham upendrakumbham merged commit df440c6 into develop Dec 13, 2024
3 checks passed
@upendrakumbham upendrakumbham deleted the bugfix/make-the-inferred-cell-type-options-dynamic-shown-or-disabled-in-marker-gene-heatmap-dropdown-menu branch December 13, 2024 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lingyun1010 lingyun1010 lingyun1010 approved these changes

@ke4 ke4 ke4 left review comments

Assignees

@upendrakumbham upendrakumbham

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@upendrakumbham @ke4 @lingyun1010