-
Notifications
You must be signed in to change notification settings - Fork 3
Add new method: CellMapper #31
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
base: main
Are you sure you want to change the base?
Add new method: CellMapper #31
Conversation
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.
Looks good code-wise from my side, but it's unclear to me how this compares to knn-smoothing.
Would need a review from @wes-lewis and/or @rcannood or @scottgigante to confirm though, as I'm not as familiar with the structure in this task.
[To address what @LuckyMD mentioned] This is quite different from the already implemented knn_smoothing approach. knn_smoothing sounds as though it would be a very general approach, but the original code from Yanai Lab uses quite specific methodology. Considering this, it should be different enough to be included. I'll describe differences a bit, as they are more specific than what is covered in the viash description (may be useful to update the description to ensure differences are accurately reflected) knn_smoothing specifics:
These aspects all differ from the choices used in CellMapper, which continues to update expression values iteratively and weights neighborhood members according to a user-defined kernel. CellMapper smoothing is more analogous to diffusion methods and less to the knn_smooth approach. I do not see any issue with the code, other than the few typos pointed our by me and @LuckyMD. I'm happy to merge as soon as these are fixed. |
Thanks for the review @LuckyMD @wes-lewis , I fixed the typos and extended the description in the config file, should be ready now. |
FYI I added myself as a contributor in the |
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.
LGTM as far as I'm concerned! @wes-lewis @LuckyMD What about you?
Describe your changes
This adds CellMapper as a new denoising method, see https://github.com/quadbio/cellmapper
Checklist before requesting a review
I have performed a self-review of my code
Check the correct box. Does this PR contain:
Proposed changes are described in the CHANGELOG.md
CI Tests succeed and look good!