Skip to content

Conversation

jan1034
Copy link
Contributor

@jan1034 jan1034 commented Apr 25, 2025

Checklist for adding packages

Mandatory

Name of the tool: PEAKQC

Short description: Periodicity Evaluation in scATAC-seq data for quality assessment

How does the package use scverse data structures (please describe in a few sentences):
The package can be used within a scanpy/episcanpy based pipeline to calculate per cell quality scores based on fragments provided by different file formats. The anndata format is used to read the barcode data and stores the results in the observables table. Scanpy functions can then be used to filter on the scores.

  • 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)
  • Automated tests cover essential functions of the package and a reasonable range of inputs and conditions [^1]
  • Continuous integration (CI) automatically executes these tests on each push or pull request [^2]
  • The package provides API documentation via a website or README[^3]
  • 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:

    • Zulip:
    • Discourse:
    • Mastodon:
    • Bluesky:
    • Twitter: @loosolab
  • The package provides tutorials (or "vignettes") that help getting users started quickly

  • The package uses the scverse cookiecutter template.

@mikkelnrasmussen
Copy link
Collaborator

Hi @jan1034,

Thanks for submitting this package! Looks like a cool tool with a well documented API. I have a few comments regarding the checklist.

We recommend that tests cover at least all user facing (public) functions. Minimal tests ensure that the function does not fail on an example data set. Ideally, tests also ensure the correctness of the results, e.g. by comparing against a snapshot.

In general, I think you’ve done a great job writing tests that cover the functions of the package. I did notice, though, that I couldn’t find test cases for the user-facing functions fld.distances_score and insertsizes.clean_chunk. Is there a reason why they cannot be tested with pytest?

Additionally, the functions fld.wavelet_transformation and the method MultithreadedMultinomialSampler.process_batch are used as helpers inside other tested functions but are not directly covered themselves. While they are helper functions, I would still recommend adding direct tests for them.

Continuous integration (CI) automatically executes these tests on each push or pull request [^2]

I could not find any continuous integration (CI) setup in your repository. Scverse ecosystem packages most commonly use GitHub Actions for CI. For an example, check out our cookiecutter template

Looking forward to hearing from you!

@grst Let me know if you have any further comments.

@jan1034
Copy link
Contributor Author

jan1034 commented May 20, 2025

Hi @mikkelnrasmussen
Thanks for reviewing my tool. Please apologize my late reply I was quiet busy during the last two weeks. I just released a new version handling the issues you found in your revision. Firstly, regarding user facing functions, I did not notice before but I had some inconsistencies regarding the naming using underscores to mark backend functions. This is now solved and insertsizes.clean_chunk() is now insertsizes._clean_chunk(), as its actually not a user facing functions. Anyway, I added the missing tests for insertsizes._clean_chunk(), MultithreadedMultinomialSampler._process_batch() .
Then I removed fld.distances_score() as its deprecated and not used anymore. Regarding fld.wavelet_transformation() it is not unit tested and not really needed to use the tool, but it was important to find the correct parameters in the development. To ensure reproducibility, I would therefore like to leave it in the repository. What do you think?
Regarding the CI, has this to be in the Github? As the Github repository is mirrored from our Gitlab, were all the pipelines are running (building enviroments, unit testing, linting, API Docs). The repository is public as well, but as it is a private hosted Gitlab. Therefore comments and issues can be opened by registered users only. Gitlab: https://gitlab.gwdg.de/loosolab/software/peakqc

Looking forward hearing from you. Thanks in advance.
Best regards,
Jan

@grst
Copy link
Contributor

grst commented May 20, 2025

Hi Jan,

let me comment on the CI topic.

Regarding the CI, has this to be in the Github? As the Github repository is mirrored from our Gitlab, were all the pipelines are running (building enviroments, unit testing, linting, API Docs). The repository is public as well, but as it is a private hosted Gitlab. Therefore comments and issues can be opened by registered users only. Gitlab: https://gitlab.gwdg.de/loosolab/software/peakqc

It is fine to rely on services different than Github for CI testing, as long as the tests are shown publicly. Thanks for sharing the link to gitlab repo, that clarifies it. I was about to suggest you add a badge linking to the test results from your README, but you already have that, so all good here!

Best,
Gregor

@mikkelnrasmussen
Copy link
Collaborator

Hi Jan,

No problem - thanks for the response! I think it sounds reasonable to keep fld.wavelet_transformation() in the repo for reproducibility and if it is not used in the tool, I think it is alright without a unit test for that function. Everything else looks good to me in regards to unit tests.

Thanks for the input on CI @grst!

Best,
Mikkel

Copy link
Collaborator

@mikkelnrasmussen mikkelnrasmussen left a comment

Choose a reason for hiding this comment

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

Concerns regarding tests and CI have been address and fulfils all other criteria. This LGTM now!

@Zethson Zethson changed the title added PEAKQC/meta.yaml Add PEAKQC May 20, 2025
@mikkelnrasmussen mikkelnrasmussen merged commit cbcfcba into scverse:main May 20, 2025
3 checks passed
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