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

Reviewer Points #63

Open
15 tasks done
jfy133 opened this issue Dec 18, 2020 · 3 comments
Open
15 tasks done

Reviewer Points #63

jfy133 opened this issue Dec 18, 2020 · 3 comments

Comments

@jfy133
Copy link
Collaborator

jfy133 commented Dec 18, 2020

Reviewer 1

  • 1. Figure 2: make names of libraries clearer in library merging plot of what each section of name refers to
  • 2. Figure 2: title of figure: check we mean schematic or scheme
  • 3. Figure 4: examples of 'real life' versions of the schematic FASTQC docs
  • 4. Line 234-235: are benchmarking results reproduced when using default values of the pipeline (without optimisation?)
    • @jfy133 Comment: I'm not entirely sure what they mean here... eager is not designed for one specific use case (human vs microbe vs animal), so I don't think it's necessary to do this as we aren't doing parameter validation.
    • @jfy133 update: spoke to the author. Above applies, but they requested should mention in text that users should always consider input parameters (e.g. for MALT screening) - they are 'sensisble' defaults but must be updated for each speicifc context - can't do everything!
  • 5. Add a pre-metagenomic screening step to remove low-complexity sequences (e.g. dustmasker, BBmsak etc)
  • 6. describe what 'more performant method' actually means for DamageProfiler
    • @jfy133 comment: it's faster and less dependencies, but the reviewer mentions rescaling which is not actually what we do here (or rather we do that with pmdtools IIRC) - but another user requested rescaling which D.P. doesn't do (AFAIK), so I suggest we add that in as an optional thing but retain D.P. for the actual damageprofiler generation as it's faster.
    • @jfy133 comment: rescaling added in Add Damage rescaling functionality nf-core/eager#642
  • 7. Two references for MultiVCFAnalyzer
    • @jfy133 comment: the second referenece is for the common challenge in bacterial palaeogenomics, not for the tool.

Reviewer 2

  • 1. Couldn't find table with alignment-based summar stats (%coverage, mean coverage, %3/5% deamination
    • @jfy133 not entirely sure what went wrong there... should be there in general stats (or reviewer didn't know about column hover for description).
    • @jfy133 update: spoke to reviewer, the columns were indeed there, they just missed them 👍
  • 2. Drop Genotyper as abandoned and replace with samtools mpileup + bcftools
  • 3. Add main text figure that provides overview of how all tools relate to each other
    • @jfy133 comment: metro map done 💪 , although I think it might make sense in methods section rather than main text.

Reviewer 3

  • 1. Link to user docs now results in 404
  • 2. provide ftp link the NCBI screening database
    • @jfy133 comment: this is a custom database from the HOPs authors unfortuately. But we'll find someway of uploading it somewhere/somehow.
  • 3. The directory containing the demonstration and benchmarking environments in the apeltzer/eager2-paper now is “content/supplement/”

General

  • Update metromap/pipeline overview to include new tools and update figures in main text accordingly
@jfy133
Copy link
Collaborator Author

jfy133 commented Dec 18, 2020

I would therefore like to propose

@ZandraFagernas would you be able to update the figure(s)?
@TCLamnidis would you be able to add mpileup + bcftools? will be added later
@aidaanva could you coordinate with getting permission and uploading the HOPS default database? @jfy133 doing as different DB
@apeltzer maybe you could add MapDamage2 rescaling functionality? @jfy133 added

I think I can do the rest...

EDIT: Ok I actually used Nucletoide Nt (that I built ;)) and not the HOPS database - no need for persmision. That's a 40GB gzipped fasta so would even fit on Zenodo 👍

@jfy133
Copy link
Collaborator Author

jfy133 commented Dec 21, 2020

mapDamage2 is on bioconda: conda install -c bioconda mapdamage2=2.2.1=pyr40_0

https://github.com/ginolhac/mapDamage

@jfy133
Copy link
Collaborator Author

jfy133 commented Jan 4, 2021

Revision submission approval from:

  • Zandra
  • Stephan
  • Aida
  • Alex
  • Maxime B.
  • Maxime G.
  • Thiseas
  • Judith

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

No branches or pull requests

1 participant