Add function to perform graph-based clustering

sjspielman commented 3 weeks ago

Purpose/implementation Section

Please link to the GitHub issue that this pull request addresses.

Closes #745

What is the goal of this pull request?

This PR adds a function to perform clustering to the rOpenScPCA package. There are many file changes, because I've also merged in main to stay in sync, but review can focus only on code in packages/rOpenScPCA.

I'm filing this as a draft to get some initial feedback, and then can implement the rest (including adding more tests, which I know are thin right now!) and mark for review. Here are the main questions I'm looking for feedback on, but of other high-level feedback is welcome :)

As a starting point (as in, we could do more/less!), I have included three arguments for specifying "additional" clustering parameters:resolution (used by louvain and leiden), objective_function (used by leiden), and cluster_args (for anything else). How does this choice feel to you? Prefer to see fewer or more of the algorithm-specific options pulled out as individual arguments?
- Noting further I don't have any checks for these arguments as of yet, but sure could!
Should the table the function returns contain additional columns, e.g. of other non-default parameters used for the clustering, beyond what I've coded up so far?
- At one point I wondered if a DataFrame would be better since it store a lot of that info as metadata, but then I felt it might get confusing for users.
Any thoughts on this comment? (noting that I edited the opening comment of #745 after I wrote this; the matrix quoted there is the wrong matrix! we want pcs, not expression...)

Author checklists

Check all those that apply. Note that you may find it easier to check off these items after the pull request is actually filed.

Analysis module and review

[ ] This analysis module uses the analysis template and has the expected directory structure.
[ ] The analysis module README.md has been updated to reflect code changes in this pull request.
[ ] The analytical code is documented and contains comments.
[ ] Any results and/or plots this code produces have been added to your S3 bucket for review.

Reproducibility checklist

[x] Code in this pull request has been added to the GitHub Action workflow that runs this module.
[ ] The dependencies required to run the code in this pull request have been added to the analysis module Dockerfile.
[ ] If applicable, the dependencies required to run the code in this pull request have been added to the analysis module conda environment.yml file.
[ ] If applicable, R package dependencies required to run the code in this pull request have been added to the analysis module renv.lock file.

sjspielman commented 2 weeks ago

Should be ready for a real look now! I think the expect_error() checks are probably overkill, but I had a nice time writing them, so here they are for now. Sorry for the clutter..........!

jashapiro commented 2 weeks ago

Should be ready for a real look now! I think the expect_error() checks are probably overkill, but I had a nice time writing them, so here they are for now. Sorry for the clutter..........!

Will review after #752 goes in

jashapiro commented 2 weeks ago

Did I say 752? I meant #753

sjspielman commented 2 weeks ago

Or we can just check that all the args have length 1, and not support args that don't follow that pattern, as I don't think we expect to use anything like that...

I haven't really been able to think of anything better than this... I guess we could also return the cluster_args list for users to do what they want with, but given that a minority of users (if any) are likely to be in this circumstance, I do think just the straight data frame return is cleaner.

sjspielman commented 2 weeks ago

Ready for another look, and in particular interested in your feedback on my docs wording about parameter defaults, and overall wording for cluster_args length-1 restriction.

sjspielman commented 2 weeks ago

I wrote a bit of what I was thinking there, but you will probably want to rewrite it.

I think what you had there was mostly there tbh, so I expanded it a little bit with more specific bluster information but kept most of what you had.

AlexsLemonade / OpenScPCA-analysis