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

Update get-started with resolutions of questions from demo #113

Merged
merged 7 commits into from
Nov 26, 2024

Conversation

falexwolf
Copy link
Member

@falexwolf falexwolf commented Nov 25, 2024

@falexwolf falexwolf changed the title Populate gene symbols In get-started, use .var to annotate genes Nov 25, 2024
@falexwolf
Copy link
Member Author

Why does this fail? I just copied from Robrecht's notebook:

image

https://lamin.ai/laminlabs/lamindata/transform/y0MIazBuljv70002

@falexwolf falexwolf changed the title In get-started, use .var to annotate genes Update get-started with questions from demo Nov 25, 2024
@falexwolf falexwolf changed the title Update get-started with questions from demo Update get-started with resolutions of questions from demo Nov 25, 2024
@falexwolf
Copy link
Member Author

falexwolf commented Nov 25, 2024

I also added an exemplary of Census to address the other important question that came up during the demo.

There are ways to make this more natively supported, but this isn't urgend. I made an issue for this:

@falexwolf
Copy link
Member Author

Can one of you please finalize this, @lazappi or @rcannood? 🙇

vignettes/laminr.Rmd Outdated Show resolved Hide resolved
@lazappi
Copy link
Collaborator

lazappi commented Nov 25, 2024

I also added an exemplary of Census to address the other important question that came up during the demo.

Does this fit in this vignette? It doesn't seem like part of the introductory workflow and doesn't use {laminr}. Could we just point someone to the documentation for the {cellxgene.census} package if they want to know how to do this?

@falexwolf
Copy link
Member Author

falexwolf commented Nov 25, 2024

Does this fit in this vignette? It doesn't seem like part of the introductory workflow and doesn't use {laminr}. Could we just point someone to the documentation for the {cellxgene.census} package if they want to know how to do this?

I agree it doesn't generally fit into this vignette, but right now the main audience for this is the person who asked the question; so it should be featured here on this page.

Once the below issue is addressed it will fit well, though:

So: Let's please keep it here for now but not for long in the present form. The section will be replaced with the $open() call in the upcoming weeks, which is fundamentally different from $load() and $cache() and hence merits to be shown in get-started.

@falexwolf
Copy link
Member Author

Can you please help to get this to pass, @lazappi? It's going to take you much less time than I. I'm simply not familiar enough.

@falexwolf falexwolf mentioned this pull request Nov 26, 2024
@lazappi
Copy link
Collaborator

lazappi commented Nov 26, 2024

I adjusted the chunk a bit but there are some bigger issues:

  1. The {cellxgene.census} package is currently not working (see [R] cellxgene-census PermanentRedirect S3 error chanzuckerberg/cellxgene-census#1261). I haven't been able to run the code manually even with the workarounds listed here. (The Python package is having similar issues DoesNotExistError: 's3://cellxgene-census-public-us-west-2/cell-census/2023-12-15/soma/' does not exist chanzuckerberg/cellxgene-census#1313)
  2. {cellxgene.census} is not available from CRAN/Bioconductor and only works on Unix. This means it is difficult for people to install and we can't really add it as a dependency so the CI will fail without some workarounds and CRAN probably won't allow it.

I think the best we could do at the moment is to have the code there but not evaluated, I'm not sure how useful that is if people can't install/use the package anyway though. There is a Bioconductor {cellxgenedp} package that could maybe be used instead but I'm not sure if it does what you are looking for.

@falexwolf
Copy link
Member Author

falexwolf commented Nov 26, 2024

Thanks, Luke!

I think the best we could do at the moment is to have the code there but not evaluated,

I agree.

I'm not sure how useful that is if people can't install/use the package anyway though

It's useful to illustrate the point of streaming a big array to get a Seurat object vs. caching a small array locally. This is important to convey to the current target audience, even if the caveats that you're listing make clear that this is currently problematic. I'm expecting these issues will get resolved by CZI though.

If it's installable on UNIX that's probably good enough. My understanding is they all work on Linux servers in the cloud.

vignettes/laminr.Rmd Outdated Show resolved Hide resolved
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

@rcannood rcannood merged commit 0a0ccb8 into main Nov 26, 2024
7 checks passed
@rcannood rcannood deleted the populatesymbols branch November 26, 2024 10:17
lazappi added a commit that referenced this pull request Nov 26, 2024
…t-open

* origin/main:
  update roadmap (#112)
  Update `get-started` with resolutions of questions from demo (#113)
lazappi added a commit that referenced this pull request Nov 28, 2024
* origin/main:
  Allow connecting to private LaminDB instances (#118)
  Use host system user for running tests (#119)
  update changelog and version
  update roadmap (#112)
  Update `get-started` with resolutions of questions from demo (#113)
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