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

migrate obsm varm tests, skip if known issue #201

Merged
merged 16 commits into from
Nov 15, 2024
Merged

Conversation

rcannood
Copy link
Collaborator

@rcannood rcannood commented Nov 14, 2024

dummy_anndata should also be able to generate data frames in obsm / varm. See data-intuitive/dummy-anndata#11

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 this should be go. I guess it's nicer, as long as the file is kept up to date.

tests/testthat/test-InMemoryAnnData.R Outdated Show resolved Hide resolved
Comment on lines +10 to +12
ad <- reticulate::import("anndata", convert = FALSE)
da <- reticulate::import("dummy_anndata", convert = FALSE)
bi <- reticulate::import_builtins()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you give these clearer names?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is ad not the standard naming convention for importing anndata -- that is,

import anndata as ad

Similarly, I thought it'd be funny if dummy_anndata would be imported as

import dummy_anndata as da

I can change the builtins, though!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I guess so, I hadn't thought of that. It's only obvious if you know the convention though and TBH I'm not a fan of doing it in Python either, I usually import things with the full name unless it's something super common like numpy. It's a reasonable thing to do though so fine to keep it.

@@ -39,62 +38,78 @@ for (name in test_names) {
adata_py$write_h5ad(file_py)

test_that(paste0("reading an AnnData with obs and var '", name, "' works"), {
msg <- message_if_known(
backend = "HDF5AnnData",
slot = c("obsp", "varp"),
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 obs/var?

Copy link
Collaborator Author

@rcannood rcannood Nov 15, 2024

Choose a reason for hiding this comment

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

Yes!

There was a bit of copy pasting. I'm considering merging the tests into one. WDYT?


Suggested change
slot = c("obsp", "varp"),
slot = c("obs", "var"),

@rcannood rcannood merged commit 2ef1304 into main Nov 15, 2024
7 checks passed
@rcannood rcannood deleted the document_known_issues branch November 15, 2024 10:07
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