add scaled MinHash/max_hash support code; many tests & minor bug fixes. #83

ctb · 2017-01-03T15:18:15Z

Adds --scaled to sourmash compute
Adds max_hash to Estimator and SourmashSignature;
Fixes bug with abundances saving/loading in Estimators;
Fixes issue with Jaccard similarity computation for empty MinHash objects;

Still need to add test to test_sourmash.py for sourmash --scaled.
address issues in Follow-up items for #79 #92

Standard checklist:

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?

codecov-io · 2017-01-03T15:35:28Z

Current coverage is 77.37% (diff: 94.77%)

Merging #83 into master will increase coverage by 0.88%

@@             master        #83   diff @@
==========================================
  Files            17         17          
  Lines          2276       2374    +98   
  Methods          48         48          
  Messages          0          0          
  Branches         85        102    +17   
==========================================
+ Hits           1741       1837    +96   
  Misses          510        510          
- Partials         25         27     +2

Powered by Codecov. Last update 4950a81...7d2dfba

luizirber · 2017-01-03T20:54:41Z

sourmash_lib/kmer_min_hash.hh

+            return;
+        }
+
+        if (!num || mins.size() < num) {


Quick comment: since we are using C++11, we can also use or instead of || and and instead of && =]

but I like ||!

Boundary try

ctb · 2017-01-05T01:00:11Z

Closes #92.

ctb · 2017-01-05T01:20:55Z

Ready for review @luizirber @lgautier. I plan to add some tests for the remaining uncovered stuff in _minhash.cc; anything else?

lgautier

Approving because none of my comments is blocking a merge. They might be worth a look though.

lgautier · 2017-01-05T01:55:58Z

sourmash_lib/__init__.py

@@ -14,6 +14,8 @@
 except ImportError:
    pass

+DEFAULT_SEED=MinHash(1,1).seed


Working but I had envisioned that the default seed could be obtained with a module-level utility (function rather than object to stress that this is read-only at the moment).

from sourmash_lib import _minhash DEFAULT_SEED = _minhash.hash_seed()

yes, I think that's cleaner, but this will at least work for the moment. If I don't get to it on this PR I'll add an issue.

lgautier · 2017-01-05T02:00:39Z

sourmash_lib/__init__.py

@@ -29,7 +31,8 @@ class Estimators(object):
    """

    def __init__(self, n=None, ksize=None, protein=False,
-                 with_cardinality=False, track_abundance=False):
+                 with_cardinality=False, track_abundance=False,
+                 max_hash=0, seed=DEFAULT_SEED):
        "Create a new MinHash estimator with size n and k-mer size ksize."


The docstring could document what each parameter is. For example I am not sure about protein, in addition to which it is passed as a value for a named parameter is_protein in a nested function call (could be called either protein or is_protein everywhere then).

done! and thanks - I'd missed that protein was different from is_protein in this code. ac43885

lgautier · 2017-01-05T02:08:36Z

sourmash_lib/__init__.py

+        return (self.num, self.ksize, self.is_protein,
+                self.mh.get_mins(with_abundance=with_abundance),
+                self.hll, self.track_abundance, self.max_hash,
+                self.seed)

    def __setstate__(self, tup):
        from . import _minhash


Would the import of _minhash at the module level (suggested in a comment above) make this unnecessary ?

lgautier · 2017-01-05T02:12:13Z

sourmash_lib/__main__.py

            # one estimator for each ksize
            Elist = []
            for k in ksizes:
                if args.protein:
                    E = sourmash_lib.Estimators(ksize=k, n=args.num_hashes,
                                                protein=True,
-                                        track_abundance=args.track_abundance)
+                                        track_abundance=args.track_abundance,


indentation issue ?

yeah... trying to keep it under 80 chr...

lgautier · 2017-01-05T02:12:33Z

sourmash_lib/__main__.py

                    Elist.append(E)
                if args.dna:
                    E = sourmash_lib.Estimators(ksize=k, n=args.num_hashes,
                                                protein=False,
                                        with_cardinality=args.with_cardinality,
-                                        track_abundance=args.track_abundance)
+                                        track_abundance=args.track_abundance,


indentation issue ?

lgautier · 2017-01-05T02:20:17Z

sourmash_lib/signature_json.py

@@ -69,6 +69,8 @@ def _json_next_signature(iterable,
    ksize = d['ksize']
    mins = d['mins']
    n = d['num']
+    max_hash = d.get('max_hash', 0)
+    seed = d.get('seed', sourmash_lib.DEFAULT_SEED)


Nice ! Elegant way to set the seed when not found in JSON file.

lgautier · 2017-01-05T02:26:53Z

sourmash_lib/test_signature.py

@@ -1,5 +1,6 @@
 import sourmash_lib
 from sourmash_lib.signature import SourmashSignature, save_signatures, load_signatures
+import math


math does not seem used anywhere in the module.

ctb · 2017-01-05T14:19:53Z

Thank you for the review!

lgautier · 2017-01-07T16:16:05Z

@ctb : I have a late question - I am realizing now that I do not understand what max_hash is for. I read and think that I understand what it is from the docstring and the C code, but not why it was introduced.

ctb · 2017-01-07T16:22:25Z

On Sat, Jan 07, 2017 at 08:16:06AM -0800, Laurent Gautier wrote: @ctb : I have a late question - I am realizing now that I do not understand what `max_hash` is for. I read and think that I understand what it is from the docstring and the C code, but not why it was introduced.

Yes, it's a research feature :). I'm still exploring its use.

lgautier · 2017-01-07T16:59:10Z

OK. Reading it as "classified information". ;-)

I am seeing here a connection with #97 in the sense that making experimentation through composability / extensibility (and without necessarily putting in the main code base) would be a good feature (although a feature of the design rather than a switch). I actually landed on this repos because I wanted to experiment with MinHash and SBT (and I am hardly using the command line, and my code is using this package as a library).

I have started looking at getting kmers/ngrams and hash together (see discussion in marbl/Mash#27), and I am using that as a opportunity / concrete use-case to suggest a bit of refactoring to make experimentations easier. I should have a pull request that, although incomplete, should help me have examples and candidate solutions to discuss relatively soon.

lgautier · 2017-01-10T19:01:06Z

Update on my earlier note:

I have started looking at getting kmers/ngrams and hash together (see discussion in marbl/Mash#27), and I am using that as a opportunity / concrete use-case to suggest a bit of refactoring to make experimentations easier. I should have a pull request that, although incomplete, should help me have examples and candidate solutions to discuss relatively soon.

While the focus was on design, but I found myself spending a lot of time thinking about how to make it fit the existing codebase. I ended rewriting from scratch and in Python. This was for the bad news.

Now the good news:

I think that I am coming up with an interesting design for library (not command line tool and interface, and I currently don't plan to).
I have cleaned a bit my implementation to make it easier to communicate about the ideas.
While more flexible (as in "it allows all kind of experimentations and try research ideas") the performances appear quite decent for a start (compared to fully C-implemented module sourmash._minhash).
I have written a short jupyter notebook to show some the points above - https://github.com/lgautier/mashing-pumpkins/blob/master/doc/notebooks/MinHash%2C%20design%20and%20performance.ipynb

ctb · 2017-01-12T17:57:43Z

On Sat, Jan 07, 2017 at 08:59:10AM -0800, Laurent Gautier wrote: OK. Reading it as "classified information". ;-)

not so much classified as "of unknown value" / "hard to explain" :)

I am seeing here a connection with #97 in the sense that making experimentation through composability / extensibility (and without necessarily putting in the main code base) would be a good feature (although a feature of the design rather than a switch). I actually landed on this repos because I wanted to experiment with MinHash and SBT (and I am hardly using the command line, and my code is using this package as a library). I have started looking at getting kmers/ngrams and hash together (see discussion in marbl/Mash#27), and I am using that as a opportunity / concrete use-case to suggest a bit of refactoring to make experimentations easier. I should have a pull request that, although incomplete, should help me have examples and candidate solutions to discuss relatively soon.

my (our) general policy on this kind of thing is that more code, and exploratory code, is fine in master as long as it's tested and doesn't mess with the existing use cases; this is where semantic versioning comes in, too, as it reassures people that command line foo remains reasonably stable. long term running branches are also very much ok, especially if they're regularly updated from master. I would prefer to avoid forks as much as possible (because then we have to make sure common bugs are communicated across the forks). for me, it's all about (a) supporting common use cases for non-/semi-developers "out of the box", (b) providing a robust platform for experimentation, and (c) progressively moving towards more used functionality and fewer bugs. none of my projects have gotten big enough that this attitude has become a problem :)

lgautier · 2017-01-13T15:43:11Z

On the same page here.

While trying to see how the writing of the shareable JSON file could be done, I saw opportunities for work on design (the alternative I saw was making the code base a bit circumvoluted) I was breaking a lot of other things in either case.

I thought about a fork, but quickly came to the same points as you (challenge to stability if merged too early, synchronization nightmare as time is passing... and if this was not enough there is an active refactoring of the C-layer to Cython).

For that reason I started from scratch with a blank page, a pencil, and an empty .py file, thinking about a very minimal minhash library (just focusing on building/using what is described here: https://en.wikipedia.org/wiki/MinHash - that's the mashing-pumpkins thing) for the time being as this has the following practical advantages:

no need to worry about breaking anything during the initial phase, thus making the trying of bold ideas easier (or try Python 3-only features)
meaningful continuous integration status with respect to unit tests and coverage
in a second phase, should there be any interesting outcomes, the integration of the ideas can be done by just using it as a library (the only code needed is an adapter at this point). Whatever bits are interesting can also be looted - i put it under a compatible license.

update: I have a number of design and implementation ideas now in mashing-pumpkins. Notfinal-final but tested and benchmarked enough to make me have a release. A proof-of-concept^* utility with that code is showing that while there is added flexibility building sourmash sketches from FASTA or FASTQ would be 2 to 3 times faster (and reportedly use much less RAM).
(*: That's proof-of-concept code, using adapter code to integrate with sourmash - that a library of building blocks).

make minhash size of 0 indicate no limit

b356744

ctb changed the title ~~make minhash max size of 0 indicate no limit to minhash size~~ [WIP] make minhash max size of 0 indicate no limit to minhash size Jan 3, 2017

ctb mentioned this pull request Jan 3, 2017

Possible inconsistency of methods of class MinHash #82

Closed

new num==0 when tracking abundances

24cc3c9

ctb added 3 commits January 3, 2017 07:58

add max_hash attribute

2191c3b

add tests for max_hash

4c869c4

add --scaled and max_hash foo

a394231

luizirber reviewed Jan 3, 2017

View reviewed changes

max_hash works with --scaled

f3a73e8

ctb mentioned this pull request Jan 4, 2017

add tests to confirm that MinHash objects are correctly created post-pickle #87

Closed

ctb added 2 commits January 4, 2017 08:16

add max_hash into pickle protocol for Estimators; test & fix abundance

5706855

test signature roundtrip w/max_hash; fix misc bugs

fef07c3

ctb mentioned this pull request Jan 4, 2017

Boundary try #91

Merged

6 tasks

ctb added 4 commits January 4, 2017 13:01

remove unneeded print statements

b9685f2

Merge pull request #91 from dib-lab/boundary_try

2eaf381

Boundary try

Merge branch 'master' of github.com:dib-lab/sourmash into boundary

f3941f7

be explicit about keyword arguments in MinHash constructor

930faa1

ctb changed the title ~~[WIP] make minhash max size of 0 indicate no limit to minhash size~~ [WIP] add scaled MinHash/max_hash support code; many tests & minor bug fixes. Jan 4, 2017

ctb added 3 commits January 4, 2017 16:37

Merge branch 'master' of github.com:dib-lab/sourmash into boundary

8b322bf

added --scaled test to test_sourmash.py

6139e55

add 'seed' into Estimator and SourmashSignature

6d2c60d

ctb added 3 commits January 4, 2017 17:00

raise ValueError for different seeds

47b2e13

added signature and sourmash cmd line tests for --seed

9ccb324

fix py2 long/float/int foo

13b99aa

test compare and concat with different seeds and max_hash

db21303

lgautier approved these changes Jan 5, 2017

View reviewed changes

convert Estimators init argument protein over to is_protein

ac43885

cleanup

7d2dfba

ctb mentioned this pull request Jan 5, 2017

clean up the way DEFAULT_SEED is set #96

Closed

ctb changed the title ~~[WIP] add scaled MinHash/max_hash support code; many tests & minor bug fixes.~~ add scaled MinHash/max_hash support code; many tests & minor bug fixes. Jan 5, 2017

ctb merged commit 2416946 into master Jan 5, 2017

ctb deleted the boundary branch January 5, 2017 14:28

This was referenced Jan 5, 2017

Follow-up items for #79 #92

Closed

Make hashing seed a parameter. #76

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add scaled MinHash/max_hash support code; many tests & minor bug fixes. #83

add scaled MinHash/max_hash support code; many tests & minor bug fixes. #83

ctb commented Jan 3, 2017 •

edited

codecov-io commented Jan 3, 2017 •

edited

luizirber Jan 3, 2017

ctb Jan 4, 2017

ctb commented Jan 5, 2017

ctb commented Jan 5, 2017

lgautier left a comment

lgautier Jan 5, 2017

ctb Jan 5, 2017

lgautier Jan 5, 2017

ctb Jan 5, 2017

lgautier Jan 5, 2017

ctb Jan 5, 2017

lgautier Jan 5, 2017

ctb Jan 5, 2017

lgautier Jan 5, 2017

lgautier Jan 5, 2017

lgautier Jan 5, 2017

ctb Jan 5, 2017

ctb commented Jan 5, 2017

lgautier commented Jan 7, 2017

ctb commented Jan 7, 2017 via email

lgautier commented Jan 7, 2017

lgautier commented Jan 10, 2017

ctb commented Jan 12, 2017 via email

lgautier commented Jan 13, 2017 •

edited

add scaled MinHash/max_hash support code; many tests & minor bug fixes. #83

add scaled MinHash/max_hash support code; many tests & minor bug fixes. #83

Conversation

ctb commented Jan 3, 2017 • edited

codecov-io commented Jan 3, 2017 • edited

Current coverage is 77.37% (diff: 94.77%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctb commented Jan 5, 2017

ctb commented Jan 5, 2017

lgautier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctb commented Jan 5, 2017

lgautier commented Jan 7, 2017

ctb commented Jan 7, 2017 via email

lgautier commented Jan 7, 2017

lgautier commented Jan 10, 2017

ctb commented Jan 12, 2017 via email

lgautier commented Jan 13, 2017 • edited

ctb commented Jan 3, 2017 •

edited

codecov-io commented Jan 3, 2017 •

edited

lgautier commented Jan 13, 2017 •

edited