Add hp encoding for proteins #758

pranathivemuri · 2019-10-22T20:31:42Z

Is it mergeable?
make test Did it pass the tests?
make coverage Is the new code covered?
Did it change the command-line interface? Only additions are allowed
without a major version increment. Changing file formats also requires a
major version number increment.
Was a spellchecker run on the source code and documentation after
changes were made?

…line while writing umi and seq in the temp fasta file

pranathivemuri · 2019-10-22T20:50:12Z

@olgabot @luizirber @taylorreiter Added HP encoding, please review when you are free!

luizirber · 2019-10-22T21:13:46Z

test_sig_filter_1 is failing in python3.5 - https://travis-ci.com/dib-lab/sourmash/jobs/248448499#L530 which I didn't change

Yes, it comes from #748, @ctb is checking it

luizirber · 2019-10-22T21:14:11Z

We should also consider #751 before merging this

pranathivemuri · 2019-10-23T23:11:33Z

I am not sure if the coverage is actually calculated correctly. The codecov bot edit history shows that coverage increased at one point yesterday, now decreased again, all I did was fix a test and add more tests and merge from master.

ctb · 2019-10-31T14:11:03Z

Thank you! I'm really enthusiastic about this functionality, and these changes look good to me. I would like to have a chance to dig into them more carefully, but might not have the time in the near future.

Questions I have:

are we saving this information in the JSON signatures?
do we have enough tests? This bit of signature calculation/comparison code is getting complicated. In particular, do we test the detection of incompatible minhash sigs?
what impact does this have on Rust branches?

pranathivemuri · 2019-10-31T17:04:35Z

Yes
I am not sure when there would be incompatible minhash signatures. currently the testing is done to see if the signature json has hp level signature, the signature contents i.e mins are not tested. They haven't been tested before either
Rust and python minhash will diverge unless we add hp encoding on rust minhash library

Thanks for reviewing and asking great questions!

luizirber · 2019-10-31T19:32:39Z

* are we saving this information in the JSON signatures?
1. Yes

We need to start thinking more about how to document these things... Adding new things to the JSON signatures should trigger data version bumps (should already have happened for dayhoff). But this is being discussed in #751 and is not really a blocker for this PR.

* what impact does this have on Rust branches?
3. Rust and python minhash will diverge unless we add hp encoding on rust minhash library

This PR is very similar to #689, and relatively straighforward to add to the Rust branches.

pranathivemuri · 2019-11-01T20:42:01Z

@luizirber I can add the rust stuff in a different PR if that's blocking this PR. Let me know if I can directly check out from master and make those changes.

luizirber · 2019-11-01T23:52:49Z

@luizirber I can add the rust stuff in a different PR if that's blocking this PR. Let me know if I can directly check out from master and make those changes.

Oh, that's not really blocking the PR. I think #751 is blocking more, but I'm comfortable with this one being merged now and we figure out the rest later.

Adding the dayhoff encoding in the Rust branch was a bit annoying because I want to keep #424 with only the Python/CFFI changes, but obviously there were changes on the Rust code too. I ended up implementing it all in the same branch to be sure it worked, and then cherry-picked changes into a new PR #760 (which contains other changes that accumulated in #424 but were not really relevant to that PR).

On @ctb comments about tests, even if we are covering the new lines I'm not so sure we are taking care of all the behaviors we expect. I'll take a closer look and come up with suggestions soon.

pranathivemuri · 2019-11-02T00:43:04Z

thanks @luizirber

Getting this error in here nf-core/kmermaid#17, at least locally: ``` == This is sourmash version 2.0.0a10.dev119+gf9bd45f. == == Please cite Brown and Irber (2016), doi:10.21105/joss.00027. == computing signatures for files: SRR4238355_subsamp_coding_reads_peptides.fasta Computing signature for ksizes: [6] Computing only Dayhoff-encoded protein (and not nucleotide) signatures. Computing a total of 1 signature(s). ... reading sequences from SRR4238355_subsamp_coding_reads_peptides.fasta Traceback (most recent call last): File "/opt/conda/envs/nfcore-kmermaid-0.1dev/bin/sourmash", line 8, in <module> sys.exit(main()) File "/opt/conda/envs/nfcore-kmermaid-0.1dev/lib/python3.6/site-packages/sourmash/__main__.py", line 83, in main cmd(sys.argv[2:]) File "/opt/conda/envs/nfcore-kmermaid-0.1dev/lib/python3.6/site-packages/sourmash/command_compute.py", line 376, in compute notify('... {} {} sequences', filename, n + 1) UnboundLocalError: local variable 'n' referenced before assignment ```

olgabot

Looks great! Had a few suggestions for documentation and comments.

sourmash/kmer_min_hash.hh

sourmash/sourmash_args.py

olgabot · 2019-11-03T17:04:12Z

tests/test__minhash.py

+    mh.add_protein(b'AGYYG')
+
+    if hp:
+        assert len(mh.get_mins()) == 1


This is still so funny to me that this sequence is all p so it only contains one k-mer!

Define 'n' before using it

add bam2fasta for requirements

pranathivemuri · 2019-11-05T22:07:40Z

requirements.txt

@@ -12,3 +12,4 @@ sphinxcontrib-napoleon
 setuptools_scm
 setuptools_scm_git_archive
 nbsphinx
+bam2fasta


maybe I should remove this, should we still keep bam2fasta optional? @olgabot @luizirber

I'll leave this up to @luizirber. I'd personally prefer to keep it there since the functionality will break if it is not installed, but not everyone needs the bam to fasta conversion.

pranathivemuri · 2019-11-06T17:46:07Z

@olgabot addressed your comments. Please review when you are free

olgabot

Thank you!

pranathivemuri and others added 30 commits October 10, 2019 11:23

add check sq false

3fedf34

Update tenx.py

f9211e8

Add whole sequence by delimiter

f761b54

add delimiter if not present

9e19dc6

edit a comment

cc5d793

update the comment merge conflict

d67968e

save_fastas flag

89013a0

save_fasta

fc41944

remove check seq false

5bed129

add test

8f58071

fix the comma in the test

edeee41

delimiter already exists

ee93292

missing barcode name

c99acb4

add a test, correct the screed fasta path

f652f41

remove umi > than symbols in the sequence output due to missing next …

4c4309e

…line while writing umi and seq in the temp fasta file

fix test

722732d

fix test

050cfc6

take mins generator out

52df53c

Write a note, and comment out the test line that randomly fails

228b6c7

save_fastas dir, not true, false flag

bfb7461

Merge branch 'master' into master

6afc0a3

convert fasta file saving to per barcode_umi

5247f6c

add more tests

aa61b08

Merge branch 'master' of https://github.com/czbiohub/sourmash

a0fe290

Add more tests

55d6d1f

fix test

2f3bbd4

Merge branch 'master' into master

ebf2b30

make tests python 2.7 compatible

e8a5860

Merge branch 'master' of https://github.com/czbiohub/sourmash

4c294ea

Merge branch 'master' into master

a8a4452

pranathivemuri and others added 4 commits October 23, 2019 08:19

Merge branch 'master' into master

759682b

Merge branch 'master' into master

69396fc

Assert correctly

5f1386e

Merge branch 'master' of https://github.com/czbiohub/sourmash

37d67a8

add paper ref for hp encoding

7a75c9f

Merge branch 'master' into master

f9bd45f

olgabot added 2 commits November 2, 2019 15:40

add bam2fasta for requirements

859bf2e

olgabot requested changes Nov 3, 2019

View reviewed changes

pranathivemuri added 4 commits November 4, 2019 11:24

Merge pull request #4 from czbiohub/olgabot-patch-1

7d5bec2

Define 'n' before using it

Merge pull request #5 from czbiohub/olgabot-patch-2

ba1530f

add bam2fasta for requirements

Edit citation, add explanation for hp

72d1f75

Merge branch 'master' of https://github.com/czbiohub/sourmash

bc9a050

pranathivemuri commented Nov 5, 2019

View reviewed changes

olgabot approved these changes Nov 7, 2019

View reviewed changes

Merge branch 'master' into master

c6b993a

luizirber mentioned this pull request Nov 22, 2019

plans for sourmash 3.0 and sourmash 4.0 #762

Closed

luizirber merged commit 0a4de16 into sourmash-bio:master Nov 22, 2019

luizirber mentioned this pull request Dec 1, 2019

Add "hp" encoding #752

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hp encoding for proteins #758

Add hp encoding for proteins #758

pranathivemuri commented Oct 22, 2019 •

edited

pranathivemuri commented Oct 22, 2019

luizirber commented Oct 22, 2019

luizirber commented Oct 22, 2019

pranathivemuri commented Oct 23, 2019

ctb commented Oct 31, 2019

pranathivemuri commented Oct 31, 2019

luizirber commented Oct 31, 2019

pranathivemuri commented Nov 1, 2019

luizirber commented Nov 1, 2019

pranathivemuri commented Nov 2, 2019

olgabot left a comment

olgabot Nov 3, 2019

pranathivemuri Nov 5, 2019

olgabot Nov 6, 2019

pranathivemuri commented Nov 6, 2019

olgabot left a comment

Add hp encoding for proteins #758

Add hp encoding for proteins #758

Conversation

pranathivemuri commented Oct 22, 2019 • edited

pranathivemuri commented Oct 22, 2019

luizirber commented Oct 22, 2019

luizirber commented Oct 22, 2019

pranathivemuri commented Oct 23, 2019

ctb commented Oct 31, 2019

pranathivemuri commented Oct 31, 2019

luizirber commented Oct 31, 2019

pranathivemuri commented Nov 1, 2019

luizirber commented Nov 1, 2019

pranathivemuri commented Nov 2, 2019

olgabot left a comment

Choose a reason for hiding this comment

olgabot Nov 3, 2019

Choose a reason for hiding this comment

pranathivemuri Nov 5, 2019

Choose a reason for hiding this comment

olgabot Nov 6, 2019

Choose a reason for hiding this comment

pranathivemuri commented Nov 6, 2019

olgabot left a comment

Choose a reason for hiding this comment

pranathivemuri commented Oct 22, 2019 •

edited