biocore / microprot

structural annotation pipeline for microbial genomes and metagenomes
BSD 3-Clause "New" or "Revised" License
1 stars 6 forks source link

Snakemake scaffold #46

Closed tkosciol closed 7 years ago

tkosciol commented 7 years ago

addressing #5 and #22

There are some issues highlighted in the code that need addressing. It also needs cleaning from hard-coded paths which were included for testing.

extra.rules need to be added to the main Snakefile for the final version.

DO NOT MERGE, but I would appreciate comments on the code.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-2.07%) to 92.193% when pulling c7012addde0f1609fc56d872515b3fc4280cdba0 on snakemake into 2410216bc2f4adcf51e1efc5b17fe0d9a2805281 on master.

tkosciol commented 7 years ago

@RNAer thanks so much for your quick comments! About log files, that's something I'm aware of. I'd like to keep all of the stdout as logs. Is there a way to do it in Snakemake other than piping to logs?

RNAer commented 7 years ago

I am not aware of. last time I checked it seems not

On Wed, Jun 7, 2017 at 8:37 PM, Tomasz notifications@github.com wrote:

@RNAer https://github.com/rnaer thanks so much for your quick comments! About log files, that's something I'm aware of. I'd like to keep all of the stdout as logs. Is there a way to do it in Snakemake other than piping to logs?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/biocore/microprot/pull/46#issuecomment-306989386, or mute the thread https://github.com/notifications/unsubscribe-auth/AA9CeJb9tS_2uR8PYF8EISpU-0fdpe7rks5sB2yKgaJpZM4NlwHv .

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.9%) to 92.697% when pulling 5752771dffd607b159769655c0980f1027199651 on snakemake into af0bf89dd4202f3815b5be32bd1983b359b43431 on master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.9%) to 92.697% when pulling 4d0e766e76404d3781e0f13c2ac617c378015cfd on snakemake into af0bf89dd4202f3815b5be32bd1983b359b43431 on master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.9%) to 92.697% when pulling 5d75810ce69f2c039852f61a3f37a923394806f9 on snakemake into af0bf89dd4202f3815b5be32bd1983b359b43431 on master.

tkosciol commented 7 years ago

I addressed most of the comments, I hope. But here's a fun problem I've got. If there is more than 1 input file (see /projects/microprot/benchmarking/snakemake_minimal_test/seq3.faa) output names are correct, but contents of each file come just from the first one. I'm trying to debug this, but having another pair of eyes would help.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.9%) to 92.697% when pulling 4eb7e4cd8ffb4c13a1ba7f12800533bdd2e2ce5e on snakemake into af0bf89dd4202f3815b5be32bd1983b359b43431 on master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.9%) to 92.697% when pulling 43da9381e10d111291424ec11fda135eb93f73b2 on snakemake into af0bf89dd4202f3815b5be32bd1983b359b43431 on master.

tkosciol commented 7 years ago

@sjanssen2 it now works fine with multiple inputs. I still haven't merged in MSA_hhblits rule into search_x, but I would call it a cosmetic improvement and have more time sensitive issues to deal with. Please, test it out on a variety of inputs and let me know your thoughts.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.9%) to 92.697% when pulling a05725850e0ba0dd15d553a9b5a3e5b98a8849af on snakemake into af0bf89dd4202f3815b5be32bd1983b359b43431 on master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-6.5%) to 88.246% when pulling 1573a6976b7c0bc0a9c11a227d37c04602e1a566 on snakemake into 4ef6ad75de2eee6f8193553bfc154e887bd68d87 on master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-6.3%) to 88.394% when pulling 81b3f6faae6fd35a691cb6703dc9ae8853e2baad on snakemake into 4ef6ad75de2eee6f8193553bfc154e887bd68d87 on master.

tkosciol commented 7 years ago

@sjanssen2 @qiyunzhu @RNAer it's not pretty, but it's good for review. Please note, this is the first version of the pipeline. So what I'm going for here it's the fact that it's working and does the job.

upcoming for future PRs:

also, write tests... I know there should be more tests, although I don't know if we can write unit tests for Snakemake itself.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-6.04%) to 88.704% when pulling a3bb6eb4aec8f0a3c29034605ad5da3864ae4021 on snakemake into 0d46d2800af0683804a12fcd77e6929719c12b5e on master.

tkosciol commented 7 years ago

@qiyunzhu comments addressed! I hope it's ready to merge.:)

qiyunzhu commented 7 years ago

@tkosciol Good job! I think it is ready to merge. Does @RNAer still request changes?

tkosciol commented 7 years ago

I think I addressed all of his requests.

Tomasz Kosciolek, PhD Postdoctoral Research Associate Rob Knight Lab University of California San Diego

On Jul 9, 2017, 3:26 PM -0700, Qiyun Zhu notifications@github.com, wrote:

@tkosciolhttps://github.com/tkosciol Good job! I think it is ready to merge. Does @RNAerhttps://github.com/rnaer still request changes?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/biocore/microprot/pull/46#issuecomment-313969651, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AGZ0Vhebwy3k6XFMmyzSel7G7N8H5Sluks5sMVOMgaJpZM4NlwHv.

qiyunzhu commented 7 years ago

@tkosciol Merged!