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

Refactor from/to Seurat conversion #183

Merged
merged 54 commits into from
Nov 12, 2024
Merged

Refactor from/to Seurat conversion #183

merged 54 commits into from
Nov 12, 2024

Conversation

rcannood
Copy link
Collaborator

@rcannood rcannood commented Sep 19, 2024

  • Redesigned the from_Seurat / to_Seurat conversion functions

@rcannood
Copy link
Collaborator Author

Note: Could use https://satijalab.github.io/seurat-object/reference/UpdateSeuratObject.html to update a Seurat <v5 object to v5 in from_Seurat().

@rcannood rcannood changed the title wip new seurat conversion Refactor from/to Seurat conversion Oct 24, 2024
This was referenced Oct 24, 2024
@scverse scverse deleted a comment from LouiseDck Nov 6, 2024
@rcannood rcannood requested review from lazappi and LouiseDck November 6, 2024 21:12
Copy link
Collaborator

@lazappi lazappi left a comment

Choose a reason for hiding this comment

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

Mostly adding suggestions for accessing stuff from a Seurat object. I didn't comment on every line but I think there should be an example for each slot.

R/Seurat.R Show resolved Hide resolved
Comment on lines 25 to 26
"To continue, install {cli::qty(missing)}{?it/them} using",
"{.code install.packages(c({missing_str}))}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

These instructions won't work for Bioconductor packages. Fixing that should probably be a separate issue. And/or maybe one about updating all messages/warning/errors to use {cli} if we are going to depend on it anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These instructions won't work for Bioconductor packages.

Fixed!

updating all messages/warning/errors to use {cli} if we are going to depend on it anyway?

I'd like that, but let's do this in a separate PR 😅

R/Seurat.R Show resolved Hide resolved
R/Seurat.R Outdated
seurat_obj[[key]] <- SeuratObject::CreateAssayObject(counts = layer_)
# trackstatus: class=Seurat, feature=get_uns, status=done
# trackstatus: class=Seurat, feature=get_varp, status=done
obj@misc <- list()

This comment was marked as resolved.

R/Seurat.R Show resolved Hide resolved
R/Seurat.R Outdated Show resolved Hide resolved
R/Seurat.R Show resolved Hide resolved
R/Seurat.R Show resolved Hide resolved
R/Seurat.R Outdated Show resolved Hide resolved
R/Seurat.R Show resolved Hide resolved
R/Seurat.R Outdated Show resolved Hide resolved
@rcannood rcannood requested review from lazappi and LouiseDck November 9, 2024 09:28
@rcannood
Copy link
Collaborator Author

rcannood commented Nov 9, 2024

Phew. I think I addressed most of the comments, except:

  • Wherever SeuratObject had functions for accessing certain data, these functions were used (e.g. changed names(obj@layers) for SeuratObject::Layers(obj)). However, I didn't change code when the alternative was less specific (e.g. changing obj@assays[[assay_name]] for obj[[assay_name]]) or readable (e.g. changing [email protected] for obj[[]]) to avoid accidental bugs.
  • Changing all stop(), warning(), cat(), etc.. to use {cli} sounds great, but let's do that in a separate PR

Also,

  • I tried to clean up with cleanup_HDF5AnnData when an error occurs while converting to HDF5AnnData. @lazappi Can you take a look?

Copy link
Collaborator

@lazappi lazappi 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 I'm happy with this now

Comment on lines 75 to 81
#' @examples
#' ad <- AnnData(
#' X = matrix(1:5, 3L, 5L),
#' obs = data.frame(row.names = LETTERS[1:3], cell = 1:3),
#' var = data.frame(row.names = letters[1:5], gene = 1:5)
#' )
#' to_Seurat(ad)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just something like what's in the documentation. I guess it depends what's in the example object.

R/Seurat.R Outdated Show resolved Hide resolved
Comment on lines +183 to +185
if (output_class == "HDF5AnnData") {
on.exit(cleanup_HDF5AnnData(...))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be outside the error part so the cleanup is still run if one of the later steps fails?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think there is no code after this call in the function. The SCE converter is simply a bit limited at this stage

Co-authored-by: Luke Zappia <[email protected]>
@rcannood rcannood merged commit 1d07188 into main Nov 12, 2024
7 checks passed
@rcannood rcannood deleted the new_seurat branch November 12, 2024 10:37
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.

3 participants