Skip to content

Simplify mapping arguments #260

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

Merged
merged 21 commits into from
Apr 29, 2025
Merged

Simplify mapping arguments #260

merged 21 commits into from
Apr 29, 2025

Conversation

lazappi
Copy link
Collaborator

@lazappi lazappi commented Apr 14, 2025

Simplify the mapping arguments in conversion functions to use named vectors instead of nested lists.

Related to (replaces?) #251, fixes #239

  • All mapping arguments now expect named vectors instead of lists
  • Complex mapping from other slots etc. is now not possible. Users will need to make these changes manually if required.
  • The exception is the reduction_mapping argument of from_Seurat()/from_SingleCellExperiment() which allows a more complex list format if users want to specify loadings etc.
  • Simple named lists will still work but are not shown in examples
  • Unnamed vectors will also work, c("item1", "item2") gets converted to c(item1 = "item1", item2 = "item2"). This is mostly to avoid simple errors and isn't shown in examples.

This helps standardise things so it will be simpler to replace from_Seurat()/from_SingleCellExperiment() with as_AnnData(). More updates to documentation will come in #253.

@lazappi lazappi marked this pull request as ready for review April 16, 2025 14:46
@lazappi lazappi requested review from rcannood and LouiseDck April 16, 2025 14:46
Copy link
Collaborator

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

LGTM. Minor suggestion

@rcannood rcannood mentioned this pull request Apr 28, 2025
1 task
Copy link
Collaborator

@LouiseDck LouiseDck 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 to me!

@@ -918,28 +947,14 @@ from_Seurat <- function(
return(invisible())
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reason to use invisible() here instead of just an early return? (purely informative)

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 functions don't return anything, they just modify adata in place. A normal return() returns NULL which might be printed to something, invisible() makes sure the result is hidden. Possibly there is a nicer way to do this.

Comment on lines +21 to +22
#' can include the `X` matrix as well. If `X` is not in the list, it will be
#' added as `counts` or `data`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be more specific: will it be added as counts or data? In what cases as counts or as data?

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 was trying not to change this here because I thought you would look at it in #255. But I can do it here if that's easier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, indeed it will be clarified in #255

@lazappi
Copy link
Collaborator Author

lazappi commented Apr 29, 2025

/document

@lazappi
Copy link
Collaborator Author

lazappi commented Apr 29, 2025

I'm going to merge this because I think it's holding up other things. I have another PR which should sort out any fixes to the documentation.

@lazappi lazappi enabled auto-merge (squash) April 29, 2025 09:00
@lazappi lazappi merged commit 3e2ba0f into main Apr 29, 2025
7 checks passed
@lazappi lazappi deleted the issue-239/simplify-mapping branch April 29, 2025 09:08
lazappi added a commit that referenced this pull request Apr 29, 2025
* origin/main:
  Simplify mapping arguments (#260)
  Update development status vignette (#262)
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.

from_Seurat: Simplify varm_mapping and other functions to length 1
3 participants