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

Polish the get-started guide #97

Merged
merged 3 commits into from
Nov 22, 2024
Merged

Polish the get-started guide #97

merged 3 commits into from
Nov 22, 2024

Conversation

falexwolf
Copy link
Member

@falexwolf falexwolf commented Nov 21, 2024

The most important change here is establishing a clearer and simpler order within the setup section.

I'd almost be tempted to install laminr after doing the python stuff, but I left it like it was for now.

@falexwolf falexwolf requested a review from rcannood November 21, 2024 19:48

This vignettes provides a quick introduction to the **{laminr}** workflow.
For more details about how **{laminr}** works see `vignette("concepts_features", package = "laminr")`.
Copy link
Member Author

Choose a reason for hiding this comment

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

This sentence now comes at the very end of the tutorial so that users don't lose time reading boilerplate.

```

Some functionality requires additional packages.
Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's super critical that connecting on the command line to configure the default instance comes now.

```

See the "Initial setup" section of `vignette("concepts_features", package = "laminr")` for more details.
This instance acts as the default instance for everything that follows.
Copy link
Member Author

Choose a reason for hiding this comment

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

I hope this is clear enough. If another sentence is needed please add.

```

# Track data provenance
# Track data lineage
Copy link
Member Author

Choose a reason for hiding this comment

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

People should get used to calling track() first thing.

@falexwolf
Copy link
Member Author

@lazappi or @rcannood, can you please take a look and merge if you like it?

@lazappi lazappi merged commit be67448 into main Nov 22, 2024
6 of 7 checks passed
@lazappi lazappi deleted the simplify branch November 22, 2024 09:56

# Start an R session
# Connect to the default instance
Copy link
Member Author

Choose a reason for hiding this comment

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

I find it problematic to say "connect to the default instance" in this heading, @lazappi, because that just happened 3 lines above on the command line. Don't you agree?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is how it reads now:

image

I felt that if one calls it "Start an R session" and then people see that they create an instance object by calling connect() in R one avoids calling two different things (CLI connect vs R connect()) with the same name "connect to the default instance".

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in new PR.

)
# Set cell identities to the provided cell type annotation
SeuratObject::Idents(seurat) <- "cell_type"
# Normalise the data
seurat <- Seurat::NormalizeData(seurat)
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that this let's peoples memory and storage explode; that's why I removed it. It'd be a bad experience if upload or compute etc. took long -- so keeping data small would be good.

If it's not actually an issue anymore, it's all good!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ignored in new PR.

@falexwolf
Copy link
Member Author

Also, I'm seeing an inconsistent level, will fix this:

image

@falexwolf
Copy link
Member Author

I will also style the UID here with backticks:

image

@falexwolf
Copy link
Member Author

Why is the default instance here cellxgene? It should be laminlabs/lamindata or laminlabs/lamin-dev, shouldn't it?

image

@falexwolf
Copy link
Member Author

Given the demo is in an hour, I'll make the cosmetic fixes just now in a new PR @rcannood @lazappi

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