Skip to content

more features to align interleaved files (with parallelization) - #25

Open
DiegoBrambilla wants to merge 16 commits into
erikrikarddaniel:masterfrom
DiegoBrambilla:master
Open

more features to align interleaved files (with parallelization)#25
DiegoBrambilla wants to merge 16 commits into
erikrikarddaniel:masterfrom
DiegoBrambilla:master

Conversation

@DiegoBrambilla

Copy link
Copy Markdown
Contributor

Hi,
There was no feature in makefile.diamond for running DIAMOND against specific databases other than RefSeq, so I created two new makefiles, makefile.diamond_bacmet and makefile.diamond_deeparg.
I have also developed rules in makefile.bowtie2 and makefile.diamond to run interleaved files.

Alsio, I have added separate recipies in makefile.blast to run both BLASTn and BLASTx with GNU Parallel.

Please check if changes conform to your standards.

@erikrikarddaniel erikrikarddaniel left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

  1. I think it would be better to keep the python scripts in their own repositories, but document how to install in relevant makefiles.

  2. I have refrained from using the .fasta file ending in rules in preference for the more specific .fna, .faa and some others. I think it would be good to stick to that and change .fasta to .fna.

  3. It looks like you deleted the header from makefile.diamond. There are more things in the diff for this one that looks like large chunks of things were deleted.

  4. You can't have an include in a library makefile, especially not one which has an absolute path! Instead, write in the documentation that one has to include some other makefiles as well.

  5. I'm not sure about having a diamond_bacmet and diamond_deeparg. It would fit better to have a makefile.bacmet and makefile.deeparg for fetching data etc. and then just continue with specifying the path to databases in individual Makefiles. (Here, you're also including makefiles...)

@DiegoBrambilla

Copy link
Copy Markdown
Contributor Author

Hi,
Back after more than one year, addressing your previous requests:

  1. I think it would be better to keep the python scripts in their own repositories, but document how to install in relevant makefiles.

The python scripts were removed from bin/. Instead, I have created dedicated gists for bacmet.py and deeparg.py, consistent with other makefiles, see for example the target interleave_all_fastq.gzs in makefile.misc. Documentation and specific make rules have been created in the dedicated makefiles to fetch such gists (see below).

  1. I have refrained from using the .fasta file ending in rules in preference for the more specific .fna, .faa and some others. I think it would be good to stick to that and change .fasta to .fna.

Done. Now rules and targets in makefile.bacmet and makefile.deeparg refer to either .fna or .faa files

  1. It looks like you deleted the header from makefile.diamond. There are more things in the diff for this one that looks like large chunks of things were deleted.

The aforementioned changes were reverted. Now the header of makefile.diamond is back to the way it was before.

  1. You can't have an include in a library makefile, especially not one which has an absolute path! Instead, write in the documentation that one has to include some other makefiles as well.

Done. include statements no longer present.

  1. I'm not sure about having a diamond_bacmet and diamond_deeparg. It would fit better to have a makefile.bacmet and makefile.deeparg for fetching data etc. and then just continue with specifying the path to databases in individual Makefiles. (Here, you're also including makefiles...)

makefile.bacmet and makefile.deeparg have been created. They have rules to fetch databases and parse DIAMOND output.
Mosty notably, they report some rules already present in makefile.diamond, namely those for the database formatting into .dmnd and the DIAMOND annotation.
The choice behind such redundancy was to generate output files that were referring to a functional database (either diamond or bacmet) through their names.
Also, you need to include only makefile.bacmet or makefile.deeparg this way if you are only interested into performing a functional annotation against BacMet or Deeparg.

@erikrikarddaniel erikrikarddaniel left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the contribution!
Many targets are duplicated and could go into e.g. makefile.misc and, I think, makefile.diamond. You can, and should, include more than one makefile and it's nothing wrong with requiring inclusion of a dependent makefile.

Comment thread lib/make/makefile.blast Outdated
NCBI_BLAST_DB_PATH = YOU NEED TO SET THE NCBI_BLAST_DB_PATH VARIABLE (see makefile.blast)
BLAST_PROTEIN_DB = YOU NEED TO SET THE BLAST_PROTEIN_DB VARIABLE (see makefile.blast)
BLAST_NUCLEOTIDE_DB = YOU NEED TO SET THE BLAST_NUCLEOTIDE_DB VARIABLE (see makefile.blast)
BLAST_NUCLEOTIDE_DB_PATH = .

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why do you need this (and the next)? Can't you use the BLAST_NUCLEOTIDE_DB variable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, will change accordingly.

Comment thread lib/make/makefile.blast Outdated
# *** Targets ***

# UNTESTED and assumes you've also included makefile.misc that can convert a fastq.gz to an fna:
all_mega.blastn6s: $(subst .fna,.mega.blastn6.tsv.gz,$(wildcard *.fastq.gz))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This will not work. You're trying to replace .fna in a list of files that ends with .fastq.gz.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, better stick with $(wildcard *.fna)

Comment thread lib/make/makefile.blast Outdated
# BLASTn-specific macros
#
# *Don't redefine!*
MAKECALL_BLASTn_VERSION = echo "$$(date +"%Y%m%d %H:%M:%S"): $@ was made with $$(blastn -version|grep 'blastn:')" > $@.makecall

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I prefer only capitals in macro names.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I understand, consistency is important.

Comment thread lib/make/makefile.bacmet Outdated
# full path of the database file, without the ".dmnd" suffix.
DIAMOND_DB =

# deeparg.py specific macro: define minimum a.a. alignemnt length threshold

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could be documented better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will do

Comment thread lib/make/makefile.bacmet Outdated
# IMPORTANT: databases are updated recurrently, so always check the availability of the http

# Download HTTPS URL for the bacmet.py script, necessary for the downstream analyses
HTTPS = https://gist.github.com/08ab264f0ea127cc242d99bc3844c743.git

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It would be better if you could package your script in Conda and instruct users to install that way. See e.g. https://github.com/erikrikarddaniel/hmmrank

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but I am short on time right now. Can we make the github gists do for the time being, please?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Fine, but I'd prefer if the instructions are to put it in ~/bin rather than in the working directory.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No problem, thanks.

Comment thread lib/make/makefile.bowtie2 Outdated
bowtie2 $(BOWTIE_ALIGN_OPTS) -x $(BOWTIE_INDEX) -1 $(word 1,$^) -2 $(word 2,$^) 2> $@.err | samtools view -Sb | samtools sort > $@
@echo "`date +"%Y%m%d %H:%M:%S"`: DONE" >> $@.makecall

%.pe.bowtie2.sorted.bam: %.r1.fastq %.r2.fastq

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You might have a use case for this, but I avoid working with unzipped fastq files as much as I can and since bowtie2 handles zipped files, I don't see why one needs this. Better to zip any files that are not zipped before starting the mapping.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rules and targets with unzipped FASTQ files will be removed.

Comment thread lib/make/makefile.deeparg Outdated
ALENGTH = 37.5

# Set this to suffixes corresponding to the types of files you wish to fetch with some
# of the t$(DIAMOND_DB)ets.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Typo?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Comment thread lib/make/makefile.deeparg Outdated
# and link the file "features.fasta" into the DIAMOND work directory
# 2) install deeparg, e.g. via conda
# $ conda create -n deeparg_env python=2.7.18
# $ source activate deeparg_env

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Should be conda activate deeparg_env.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

Comment thread lib/make/makefile.deeparg Outdated
# *** misc targets ***

# compress all .fastq files in the work directory.
compress_all_fastqs: $(subst .fastq,.fastq.gz, $(wildcard *.fastq))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Typical target for makefile.misc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed: better left on makefile.misc

Comment thread lib/make/makefile.deeparg Outdated
# Dependencies will automatically be unzipped if not present in unzipped format
# thanks to the pattern above. Unzipped files will be deleted after
# interleaving.
interleave_all_fastq.gzs: $(subst .R1.fastq.gz,.intlv.fastq.gz,$(wildcard *.R1.fastq.gz))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Also makefile.misc. (Also in another file I reviewed above. Define every target only once.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as above reply.

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