MinHash.merge() modifies signature inplace -- is it supposed to?

[phiweger]

Example:

# load one signature "sig"
mh = MinHash(ksize=21, scaled=100, n=0)
assert len(mh) == 0

newmh = mh.merge(sig.minhash)
assert len(mh) == 0
AssertionError:

Is this behaviour intended?

[ctb]

I see the same behavior, and I agree it feels inconsistent; in the code base we have both behaviors being used, so I will need to review the impact of changing this... Thanks for the catch!

[phiweger]

I mean inplace can be useful when you merge many sigs, but then the return value should be None. Anyway, thanks for looking into this.

[ctb]

On Tue, Oct 13, 2020 at 12:23:39PM -0700, Adrian Viehweger wrote: I mean inplace can be useful when you merge many sigs, but then the return value should be None. Anyway, thanks for looking into this.

exactly right :) The += operator should work, BTW, and may be clearer in your code in this case.

[ctb]

FYI, in #1282 I am explicitly endorsing the in-place behavior of merge, and providing addition functionality via the __add__ operator, which I think is closer to what you want here.