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

Minor Changes to the pipeline #2

Merged
merged 29 commits into from
Aug 4, 2023
Merged

Minor Changes to the pipeline #2

merged 29 commits into from
Aug 4, 2023

Conversation

SaimMomin12
Copy link
Contributor

No description provided.

Copy link
Member

Choose a reason for hiding this comment

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

looks great, three minor suggestions:

  • pandas / snakemake could be wrapped in sc-virome-scan 'base' environment (where the workflow will run from). There is env.yaml in the rootdir, though this seems to be rule specific already ?
  • perhaps a --lint run (can this be combined with --dryrun) ?
  • rather then flake check the name could be something related to dryrun

READMD.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

duplicate of README.md I think ?

Copy link
Member

Choose a reason for hiding this comment

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

see comment on workflow, this env seems redundant with tools / kraken2.yaml ?

Copy link
Member

Choose a reason for hiding this comment

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

could this be packed with tools.yaml ?

Copy link
Member

Choose a reason for hiding this comment

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

Could you omit --chemistry ? Auto detection should take care of this, right ?

Copy link
Member

Choose a reason for hiding this comment

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

quite some potential to optimise this, though let's tackle this later.

Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be a rule ?

Copy link
Member

Choose a reason for hiding this comment

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

This function should be anonymised (e.g. os.expanduser() )

Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be in the repo I think.

Copy link
Member

@WardDeb WardDeb left a comment

Choose a reason for hiding this comment

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

Looks good, I'd make a couple changes before moving on:

  • clean the duplicated conda envs, not sure what is being used / what is redundant
  • remove the test_download.sh

@WardDeb WardDeb merged commit 32a2895 into main Aug 4, 2023
1 check 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.

2 participants