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.

Sign up for GitHub

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

Jump to bottom

KBTools update to nf-core module #126

Merged
merged 4 commits into from Jul 5, 2022
Merged

KBTools update to nf-core module #126

merged 4 commits into from Jul 5, 2022

Conversation

apeltzer
Copy link
Member

@apeltzer apeltzer commented Jul 5, 2022

This:

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
    • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
    • If necessary, also make a PR on the nf-core/scrnaseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions
Copy link

github-actions bot commented Jul 5, 2022

This PR is against the master branch ❌

  • Do not close this PR
  • Click Edit and change the base to dev
  • This CI test will remain failed until you push a new commit

Hi @apeltzer,

It looks like this pull-request is has been made against the nf-core/scrnaseq master branch.
The master branch on nf-core repositories should always contain code from the latest release.
Because of this, PRs to master are only allowed if they come from the nf-core/scrnaseq dev branch.

You do not need to close this PR, you can change the target branch to dev by clicking the "Edit" button at the top of this page.
Note that even after this, the test will continue to show as failing until you push a new commit.

Thanks again for your contribution!

@apeltzer apeltzer changed the base branch from master to dev July 5, 2022 11:40
@github-actions
Copy link

github-actions bot commented Jul 5, 2022 •
edited

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit f3ce79c

+| ✅ 148 tests passed       |+
!| ❗   1 tests had warnings |!

❗ Test warnings:

  • readme - README did not have a Nextflow minimum version badge.

✅ Tests passed:

Run details

  • nf-core/tools version 2.4.1
  • Run at 2022-07-05 12:03:18

grst
grst approved these changes Jul 5, 2022
View reviewed changes
Copy link
Member

@grst grst left a comment

Choose a reason for hiding this comment

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

LGTM

fmalmeida
fmalmeida approved these changes Jul 5, 2022
View reviewed changes
Copy link
Contributor

@fmalmeida fmalmeida left a comment •
edited

Choose a reason for hiding this comment

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

Looking just at the changes themselves, everything looks good. I would just say to see if the pipeline works properly before merging. Otherwise, all looks good.

@apeltzer
Copy link
Member Author

apeltzer commented Jul 5, 2022

@fmalmeida - yeah there is one thing missing:

Pulling from biocontainers/scanpy
2022-07-05T11:46:14.6086127Z   cefc4d495539: Pulling fs layer
2022-07-05T11:46:14.6088668Z   4ca545ee6d5d: Pulling fs layer
2022-07-05T11:46:14.6088928Z   25207212085e: Pulling fs layer
2022-07-05T11:46:14.6090796Z   4ca545ee6d5d: Verifying Checksum
2022-07-05T11:46:14.6094176Z   4ca545ee6d5d: Download complete
2022-07-05T11:46:14.6099221Z   cefc4d495539: Verifying Checksum
2022-07-05T11:46:14.6102910Z   cefc4d495539: Download complete
2022-07-05T11:46:14.6105215Z   cefc4d495539: Pull complete
2022-07-05T11:46:14.6107215Z   4ca545ee6d5d: Pull complete
2022-07-05T11:46:14.6109498Z   25207212085e: Download complete
2022-07-05T11:46:14.6111593Z   25207212085e: Pull complete
2022-07-05T11:46:14.6112227Z   Digest: sha256:9cbc28b321d7c930c930ab871bab9c8c691cdf52c32e0e4995951284e609ad2e
2022-07-05T11:46:14.6114813Z   Status: Downloaded newer image for quay.io/biocontainers/scanpy:1.7.2--pyhdfd78af_0
2022-07-05T11:46:14.6115204Z   Traceback (most recent call last):
2022-07-05T11:46:14.6118708Z     File "/home/runner/work/scrnaseq/scrnaseq/bin/mtx_to_h5ad.py", line 40, in <module>
2022-07-05T11:46:14.6119457Z       adata = mtx_to_adata(
2022-07-05T11:46:14.6122271Z     File "/home/runner/work/scrnaseq/scrnaseq/bin/mtx_to_h5ad.py", line 14, in mtx_to_adata
2022-07-05T11:46:14.6122660Z       adata = sc.read_mtx(mtx_file)
2022-07-05T11:46:14.6123133Z     File "/usr/local/lib/python3.8/site-packages/anndata/_io/read.py", line 282, in read_mtx
2022-07-05T11:46:14.6125731Z       X = mmread(fspath(filename)).astype(dtype)
2022-07-05T11:46:14.6126247Z     File "/usr/local/lib/python3.8/site-packages/scipy/io/mmio.py", line 72, in mmread
2022-07-05T11:46:14.6128210Z       return MMFile().read(source)
2022-07-05T11:46:14.6129083Z     File "/usr/local/lib/python3.8/site-packages/scipy/io/mmio.py", line 422, in read
2022-07-05T11:46:14.6129483Z       stream, close_it = self._open(source)
2022-07-05T11:46:14.6135668Z     File "/usr/local/lib/python3.8/site-packages/scipy/io/mmio.py", line 324, in _open
2022-07-05T11:46:14.6139289Z       stream = open(filespec, mode)
2022-07-05T11:46:14.6141935Z   FileNotFoundError: [Errno 2] No such file or directory: '*_kallistobustools_count/counts_unfiltered/*.mtx'
2022-07-05T11:46:14.6144247Z 
2022-07-05T11:46:14.6147115Z Work dir:
2022-07-05T11:46:14.6149136Z   /home/runner/work/scrnaseq/scrnaseq/work/c2/a387f64b7930970643cbc6eaf69cd5```

In direct relation to the *.mtx conversion - any idea? Something missing from the outputs apparently.

@fmalmeida
Copy link
Contributor

I saw an error on the test that we need to fix before merging. Now that the module is from nf-core, it seems that the .mtx file is not stored in the same path anymore, thus the conversion steps failed.

@fmalmeida
Copy link
Contributor

fmalmeida commented Jul 5, 2022 •
edited

we saw the same thing at the same time 🤣

I will check what is it.

@apeltzer
Copy link
Member Author

apeltzer commented Jul 5, 2022

Running the small tests locally on gitpod, lets see.

@apeltzer
Copy link
Member Author

apeltzer commented Jul 5, 2022 •
edited

Looks like the new module doesn't stage the same as the old local one:

Old:

output:
    tuple val(meta), path ("*_kallistobustools_count*") , emit: counts
    path  "versions.yml"                                , emit: versions

New:

output:
    tuple val(meta), path ("*.count"), emit: count
    path "versions.yml"              , emit: versions

The former includes way more files than the latter, the glob looks quite different.

@fmalmeida
Copy link
Contributor

Looks like the new module doesn't stage the same as the old local one:

Old:

output:
    tuple val(meta), path ("*_kallistobustools_count*") , emit: counts
    path  "versions.yml"                                , emit: versions

New:

output:
    tuple val(meta), path ("*.count"), emit: count
    path "versions.yml"              , emit: versions

so there it is. This is the problem.

@apeltzer
Copy link
Member Author

apeltzer commented Jul 5, 2022

Will open a PR to update the nf-core module accordingly to fix this, we need to review + merge then, then update here again and it should auto-fix things.

@apeltzer
Copy link
Member Author

apeltzer commented Jul 5, 2022

Hm, not really. The new module uses this as output prefix:

-o ${prefix}.count - so the glob is in fact correct

@apeltzer
Copy link
Member Author

apeltzer commented Jul 5, 2022

@apeltzer apeltzer merged commit dfc9b43 into dev Jul 5, 2022
@apeltzer apeltzer deleted the kbtools-fix branch July 5, 2022 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grst grst grst approved these changes

@fmalmeida fmalmeida fmalmeida approved these changes

@ggabernet ggabernet Awaiting requested review from ggabernet

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing options c1 and c2 with kallistobustools count
3 participants
@apeltzer @fmalmeida @grst