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

Extract PC matrix within clustering function (2/2) #759

Conversation

sjspielman
Copy link
Member

Closes #754

This PR does the second half of #754 and allows calculate_clusters() to take a single-cell object instead of only a matrix.
I'm opening this as a draft because I couldn't pick between two ways of doing this..so I implemented them both. Which general approach should we go with (noting it might be a little bit "why not both"!)?

  • First, in 01d538a, I primarily updated the calculate_clusters() function and left the extract pc function alone. While writing docs for it though, it felt odd strange to describe the default pc_name argument behavior in the clustering function when the extract function was actually defining those defaults. I then wondered if too much was getting duplicated, so I then...
  • Implemented the most recent commit shown in the PR, where I moved a decent amount of the behavior out of the extract function and into the clustering function. Here, the extract function requires all arguments.

Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

I think you may have overcomplicated things! The extract function you had already did all the conversions and checks, so let it handle that. I made a suggestion that I feel like ought to work with pretty minimal code changes, if you revert the extract function to its previous form.

packages/rOpenScPCA/R/calculate-clusters.R Outdated Show resolved Hide resolved
packages/rOpenScPCA/R/calculate-clusters.R Outdated Show resolved Hide resolved
packages/rOpenScPCA/R/calculate-clusters.R Outdated Show resolved Hide resolved
@sjspielman
Copy link
Member Author

sjspielman commented Sep 12, 2024

I think you may have overcomplicated things!

Me?!

But in all seriousness, yes. What you have suggested is more or less my first commit, so I'll just revert and get back on track there!

…alculate_clusters and out of extract_pc_matrix"

This reverts commit 22b15fa.
@jashapiro
Copy link
Member

  • While writing docs for it though, it felt odd strange to describe the default pc_name argument behavior in the clustering function when the extract function was actually defining those defaults.

Just write something like "optional pc_name argument passed to extract_pc_matrix"

@sjspielman sjspielman marked this pull request as ready for review September 16, 2024 14:24
@sjspielman sjspielman removed the request for review from jaclyn-taroni September 16, 2024 14:24
@sjspielman
Copy link
Member Author

Should be ready for a real look now. Note that I ended up exporting extract_pc_matrix (so I also wrote some quick examples) since we refer to it in the calculate_cluster docs now.

Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

LGTM, with one little comment about column name checks.

sjspielman and others added 2 commits September 16, 2024 13:57
@sjspielman sjspielman merged commit 1b4e487 into AlexsLemonade:feature/ropenscpca Sep 16, 2024
3 checks passed
@sjspielman sjspielman deleted the 754-flexiblize-calculate-clusters branch September 16, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants