search

tseemann / snp-dists

Pairwise SNP distance matrix from a FASTA sequence alignment
GNU General Public License v3.0
127 stars 28 forks source link

-t option for headers with molten #48

Open matt-sd-watson opened 3 years ago

matt-sd-watson commented 3 years ago

I thought it may be useful to have the option for the output/output file generated with molten format to be able to have column headers. I use snp-dists in COVID-19 analysis pipelines and often use the snp-dists molten output to filter for sample subsets based on SNP distance. Having defined column headers may make it easier to read in and filter these molten outputs.

The column headers can be added with '-t' when '-m' is also enabled.

Let me know if the PR is not appreciated, thanks for the tool!

matt-sd-watson commented 3 years ago

Hi @kloetzl, let me know if any other changes should be made for this PR, thanks!

kloetzl commented 3 years ago

I leave it to Thorsten to make the call whether to merge this. I'm only wondering if -t is the right option. Maybe -H for header is better? Or combine it with one of the others?

matt-sd-watson commented 3 years ago

Thanks for the feedback, I appreciate it! I wasn't entirely sure which option would be best since -h was already used. I guess my intuition was that -t could be associated with "top" or "title" for the molten header, but I am happy to change it if another option seems more appropriate. Generally I thought that it would be nice to have the option for a header or not with molten, but if it's not appreciated I understand!

kloetzl commented 3 years ago

I agree, a header is nice, but is also easy to add one via standard unix tools:

snp-dists foo.fa | cat <(echo "seq_1\tseq_2\tdist") -
matt-sd-watson commented 3 years ago

Fair enough! If you and Torsten feel that it's not necessary then I can close the PR.

kloetzl commented 3 years ago

Generally I thought that it would be nice to have the option for a header or not with molten, but if it's not appreciated I understand!

If that is the impression you got I need to apologize. I would have merged this pull request almost straight away, I just don't have the permissions to do it. We need to wait for @tseemann to do that.

matt-sd-watson commented 3 years ago

Generally I thought that it would be nice to have the option for a header or not with molten, but if it's not appreciated I understand!

If that is the impression you got I need to apologize. I would have merged this pull request almost straight away, I just don't have the permissions to do it. We need to wait for @tseemann to do that.

No problem at all @kloetzl! I know that it's a very trivial PR so I understand that there is no specific need to merge this in now.