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

[r] Add drop_levels to SOMAExperimentAxisQuery -> ecosystem outgestors #2825

Conversation

mojaveazure
Copy link
Member

R analog of #2811 and single-cell-data/SOMA#204; add a drop_levels paramter to the ecosystem outgestors to drop unused factor levels from resulting data frames

Modified SOMA methods:

  • SOMAExperimentAxisQuery$to_seurat(): add drop_levels to drop drop unused levels from obs and var data frames
  • SOMAExperimentAxisQuery$to_seurat_assay(): add drop_levels to drop unused levels from var data frame
  • SOMAExperimentAxisQuery$to_single_cell_experiment(): add drop_levels to drop unused levels from obs and var data frames

Also shifts SOMAExperimentAxisQuery$to_seurat() and SOMAExperimentAxisQuery$to_seurat_assay() to use SOMAExperimentAxisQuery$private$.load_df() for loading obs and var; removing standalone code and increase sharing with the SCE outgestor

resolves #2765

SC-51945

…stors

R analog of #2811 and single-cell-data/SOMA#204; add a `drop_levels`
paramter to the ecosystem outgestors to drop unused factor levels from
resulting data frames

Modified SOMA methods:
 - `SOMAExperimentAxisQuery$to_seurat()`: add `drop_levels` to drop
   drop unused levels from `obs` and `var` data frames
 - `SOMAExperimentAxisQuery$to_seurat_assay()`: add `drop_levels` to
   drop unused levels from `var` data frame
 - `SOMAExperimentAxisQuery$to_single_cell_experiment()`: add
   `drop_levels` to drop unused levels from `obs` and `var` data frames

Also shifts `SOMAExperimentAxisQuery$to_seurat()` and
`SOMAExperimentAxisQuery$to_seurat_assay()` to use
`SOMAExperimentAxisQuery$private$.load_df()` for loading `obs` and
`var`; removing standalone code and increase sharing with the SCE
outgestor

resolves #2765

[SC-51945](https://app.shortcut.com/tiledb-inc/story/51945)
@@ -515,6 +515,7 @@ SOMAExperimentAxisQuery <- R6::R6Class(
#' @param obsm_layers \Sexpr[results=rd]{tiledbsoma:::rd_outgest_mlayers()}
#' @param varm_layers \Sexpr[results=rd]{tiledbsoma:::rd_outgest_mlayers(axis = 'varm')}
#' @param obsp_layers \Sexpr[results=rd]{tiledbsoma:::rd_outgest_players()}
#' @param drop_levels Drop unused levels from \code{obs} and \code{var} factor columns
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not use these patterns when I write docs so pardon the naive question but why do all other params here follow a pattern and this one doesn't?

Copy link
Member Author

Choose a reason for hiding this comment

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

The previous ones used \Sexpr for templated docs that are similar but with minor variations between different methods as @template does not work with R6; however, the use of \Sexpr is the cause of the "installing package to process help pages" step when building tiledbsoma-r from source. As this step has been a source of complaint, I've resorted to copy-pasting docs and will eventually switch out all \Sexpr to copy-pasted docs to make building tiledbsoma-r easier

Copy link
Contributor

Choose a reason for hiding this comment

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

I am 100% on board with the simplification and robustification.

Copy link
Contributor

@eddelbuettel eddelbuettel 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 per the added tests.

One mostly curiously question about Rd writing style inline

Bump develop version

[ci skip]
@mojaveazure mojaveazure merged commit d6e0719 into main Aug 2, 2024
@mojaveazure mojaveazure deleted the paulhoffman/sc-51945/to-seurat-and-to-single-cell-experiment-need branch August 2, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants