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

new package: sincei #177

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

new package: sincei #177

wants to merge 2 commits into from

Conversation

vivekbhr
Copy link

Name of the tool: sincei

Short description: A user-friendly toolkit for QC, counting, clustering and plotting of single-cell (epi)genomics data

How does the package use scverse data structures (please describe in a few sentences): stores information in anndata format

  • The code is publicly available under an OSI-approved license
  • The package provides versioned releases
  • The package can be installed from a standard registry (e.g. PyPI, conda-forge, bioconda)
  • The package uses automated software tests and runs them via continuous integration (CI)
  • The package provides API documentation via a website or README
  • The package uses scverse datastructures where appropriate (i.e. AnnData, MuData or SpatialData and their modality-specific extensions)
  • I am an author or maintainer of the tool and agree on listing the package on the scverse website

Recommended

  • Please announce this package on scverse communication channels (zulip, discourse, twitter)
  • Please tag the author(s) these announcements. Handles (e.g. @scverse_team) to include are:
    • Twitter: @vivekbhr
    • Zulip:
    • Discourse:
    • Mastodon:
  • The package provides tutorials (or "vignettes") that help getting users started quickly
  • The package uses the scverse cookiecutter template.

@grst
Copy link
Contributor

grst commented Jul 2, 2024

Hi @vivekbhr,

thanks for submitting the package! Looks good overall, I have a few specific comments

The package can be installed from a standard registry (e.g. PyPI, conda-forge, bioconda)

  • The package is available from PyPI - why do you recommend to install the stable version via pip install git+... in the README?

The package uses scverse datastructures where appropriate (i.e. AnnData, MuData or SpatialData and their modality-specific extensions)

Here I see some room for improvement... In particular, the API is not very consistent here.

  • GLMPCA doesn't take an AnnData object, but rather matrix
  • TOPICMODEL takes a sparse matrix and a list of cell indices, while it could take AnnData + a column name in anndata.obs
  • multimodalClustering does take AnnData objects already. Since it deals with multimodal data, it would be natural to support MuData input. It also returns anndata objects and umap coordinates separately, while it would be nice to write umap coordinates in .obsm and cluster labels in .obs for plotting with scanpy.

It is completely fine to have a more scikit-learn like interface that works on matrices rather than anndata objects, as long as it is shown in the tutorial how this works with scverse data structures, e.g.

adata.obsm["glmpca"] = GLMPCA.fit_transform(adata.layers["raw_counts"])

But my criticism here is that (a) it's not consistent and (b) the Python tutorial doesn't embrace scverse data structures at all.

The package provides API documentation via a website or README

There's a lot of functions in the API documentation that are not fully documented (no description on the arguments and the return values). If they are not meant to be user-facing, consider making them private and removing them from the documentation. If they are meant to be used, please document them.

@vivekbhr
Copy link
Author

vivekbhr commented Jul 2, 2024

@grst Thank you for the review. Indeed, the python API was not designed to read/write anndata objects. But the CLI wrappers do store everything in anndata. I agree we can improve the tutorial to indicate how to use anndata input, like in your example.

We will work on the improvements and get back to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants