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 README and small fixes #130

Merged
merged 16 commits into from
Jun 19, 2024
Merged

Update README and small fixes #130

merged 16 commits into from
Jun 19, 2024

Conversation

luisas
Copy link
Collaborator

@luisas luisas commented Jun 6, 2024

Update README, small fixes and fix tests

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/multiplesequencealign 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>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,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).

Copy link

github-actions bot commented Jun 6, 2024

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 991bc91

+| ✅ 193 tests passed       |+
#| ❔  21 tests were ignored |#
!| ❗   3 tests had warnings |!

❗ Test warnings:

  • readme - README contains the placeholder zenodo.XXXXXXX. This should be replaced with the zenodo doi (after the first release).
  • pipeline_todos - TODO string in README.md: Add citation for pipeline after first release. Uncomment lines below and update Zenodo doi and badge at the top of this file.
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-06-19 09:40:43

@luisas luisas requested a review from JoseEspinosa June 6, 2024 15:19
Copy link
Member

@JoseEspinosa JoseEspinosa left a comment

Choose a reason for hiding this comment

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

Some small suggestions, but otherwise awesome job!

README.md Outdated Show resolved Hide resolved
README.md Outdated
Each row represents a set of sequences (in this case the seatoxin and toxin protein families) to be processed.

`id` is the name of the set of sequences. It can correspond to the protein family name or to an internal id.
Each row represents a set of sequences (in this case the seatoxin and toxin protein families) to be aligned.
Copy link
Member

Choose a reason for hiding this comment

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

I think that explaining what each column represents (even if they are optional) as before would be clearer for users

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was following the rnaseq structure and in the main they keep it super minimal and specify the columns in usage and this is what i tried to do - i could copy it here from usage but then it is maybe a bit redundant? I added some overview on the columns here bt not as secific as usage - let me know aht you think!

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough

README.md Outdated Show resolved Hide resolved
Comment on lines 31 to 32
// Output directory
outdir = "./outdir/"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Output directory
outdir = "./outdir/"

Comment on lines 32 to 33
// Output directory
outdir = "./outdir/"
Copy link
Member

Choose a reason for hiding this comment

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

outdir has to be implicitly provided since this won't in aws for instance that is why it has become a mandatory parameter, please also remove it from test.config

Suggested change
// Output directory
outdir = "./outdir/"

docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
```

### Multiple runs of the same sample
Each row represents a set of sequences (in this case the seatoxin and toxin protein families) to be processed.
Copy link
Member

Choose a reason for hiding this comment

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

this is not generic? I mean the example is still for "seatoxin and toxin protein families"?

Copy link
Member

Choose a reason for hiding this comment

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

or maybe I am missing something, before we have two examples and now only one, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sisi it is for 2 families, like in the main readme, but now we go into details on what each column is etc

@@ -1,8 +1,8 @@
process MULTIQC {
label 'process_medium'

conda 'bioconda::multiqc=1.22.1'
container "community.wave.seqera.io/library/pip_multiqc:2c2e276ad8997cc4"
conda 'bioconda::multiqc=1.22.2'
Copy link
Member

Choose a reason for hiding this comment

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

couldn't we use the modules implementation of multiqc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No because i have to pass specific inputs that the normal one does not take - same as in rnaseq local multiqc

workflows/multiplesequencealign.nf Outdated Show resolved Hide resolved
@luisas
Copy link
Collaborator Author

luisas commented Jun 19, 2024

closes #85

Copy link
Member

@JoseEspinosa JoseEspinosa left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! 🚀

@luisas luisas merged commit 714aa45 into nf-core:dev Jun 19, 2024
16 checks passed
@luisas luisas deleted the luisa_patch branch June 19, 2024 12:58
@luisas luisas mentioned this pull request Jun 19, 2024
3 tasks
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