Some hacking on labelhash. #1021

ctb · 2015-05-21T03:00:13Z

@camillescott will "like" this, I suspect... for some value of "like".

If this branch proves useful we should make the Python type hierarchy for Hashtable/CountingHash/Hashbits look more like the C++ object hierarchy. But let's see if it's useful first.

…ubclass

camillescott · 2015-05-21T03:07:04Z

Ha! I was literally thinking about doing this EXACT thing today during the meeting at some point when I was, erm, clearly not going on a brain tangent. Not sure how you've developed these new mind-probing powers, but be careful in my head, it's perilous in there...

So yes, I like it. Note though: wouldn't it be better to just pass LabelHash a Hashbits or CountingHash from pythonland, rather than having it construct a new one in the __new__ method? That would also avoid having to override the LabelHash class at all in __init__.py

ctb · 2015-05-21T03:08:55Z

On Wed, May 20, 2015 at 08:07:04PM -0700, Camille Scott wrote:

Ha! I was literally thinking about doing this EXACT thing today during the meeting at some point when I was, erm, clearly not going on a brain tangent. Not sure how you've developed these new mind-probing powers, but be careful in my head, it's perilous in there...

No Comment.

So yes, I like it. Note though: wouldn't it be better to just pass LabelHash a Hashbits or CountingHash from pythonland, rather than having it construct a new one in the `new

Yep.

Now leave me alone, I've got hacking to do :)

camillescott · 2015-05-21T03:10:09Z

Very well, carry on :)

ctb · 2015-05-21T03:13:05Z

While you are on the line -

if I'm loading labels and tags from disk, all I need to do is run

 check_and_allocate_label(label)

once for each label and then

 link_tag_and_label(tag, label)

for each tag/label pair, correct?

camillescott · 2015-05-21T03:15:44Z

Yerp, that should do it.

ctb · 2015-05-21T04:25:53Z

Huh. Somewhat to my surprise, that seems to work.

mr-c · 2015-05-26T18:03:31Z

khmer/_khmermodule.cc

+        }
+        else if (PyObject_TypeCheck(hashtable_o, &khmer_KCountingHash_Type)) {
+          khmer_KCountingHash_Object * cho = (khmer_KCountingHash_Object *) hashtable_o;
+          hashtable = cho->counting;
        }


Needs a final else that sets an error and returns.

…y tests

…raph

ctb · 2015-05-26T22:47:02Z

Is it mergeable?
Did it pass the tests?
If it introduces new functionality in scripts/ is it tested?
Check for code coverage with make clean diff-cover
Is it well formatted? Look at make pep8, make diff_pylint_report,
make cppcheck, and make doc output. Use make format and manual
fixing as needed.
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.
Is it documented in the ChangeLog?
http://en.wikipedia.org/wiki/Changelog#Format
Was a spellchecker run on the source code and documentation after
changes were made?
Is the Copyright year up to date?

ctb · 2015-05-26T22:51:29Z

The tests failed because of lowered code coverage, due to removal of so many lines of code. @mr-c, could you update the code coverage requirements, please, and then review?

mr-c · 2015-05-27T14:35:09Z

khmer/__init__.py

+        return c
+
+
+class CountingLabelHash(_LabelHash):


This class has no test coverage.

mr-c · 2015-05-27T14:39:08Z

lib/labelhash.cc

+        infile.read((char *) &version, 1);
+        infile.read((char *) &ht_type, 1);
+        if (!(version == SAVED_FORMAT_VERSION)) {
+            std::ostringstream err;


This branch has no code coverage

mr-c · 2015-05-27T14:50:28Z

Code coverage was reduced due to new function and code paths added without tests :-)

ctb · 2015-05-28T01:20:34Z

retest this please, jenkins

ctb · 2015-05-28T01:25:23Z

Hmm @mr-c looks like a Jenkins problem - out of memory in unrelated tests. Any thoughts?

Anyway, the PR is ready for review.

mr-c · 2015-05-28T01:35:50Z

Maybe your test creates a memory leak prior?

ctb · 2015-05-28T01:37:27Z

On Wed, May 27, 2015 at 06:35:51PM -0700, Michael R. Crusoe wrote:

Maybe your test creates a memory leak prior?

Maybe? But can't think of what it would be...

ctb · 2015-05-28T01:38:11Z

oh. That. Figured it out.

ctb · 2015-05-28T01:40:37Z

huh, wonder why we dynamically allocate buf :)

ctb · 2015-05-28T01:47:25Z

whew ready for review @mr-c.

mr-c · 2015-05-28T19:32:13Z

@ctb Lookin' good. I found a couple small things via cpychecker but nothing from your commits.

Some hacking on labelhash.

ctb added 3 commits May 20, 2015 21:56

first round refactor of LabelHash to be a friend class instead of a s…

29afe16

…ubclass

passes tests

3181ba7

smushed in the KCountingHash object

6f8b6e2

created new CountingLabelHash class

7c9545b

ctb added 2 commits May 20, 2015 23:49

save & load

55f55f0

fix load/save

328f686

fix aberrant pointer storage

caf7f3e

mr-c reviewed May 26, 2015
View reviewed changes

ctb added 7 commits May 26, 2015 18:13

renamed hashbits attribute on LabelHash to .graph; removed unnecessar…

b6ef20e

…y tests

renamed _ht member to 'graph' on LabelHash

0fd0751

added tests for load/save_labels_and_tags

8526e97

remove pep8 violation/unneeded code

55def20

added error checking for incorrect graph type at LabelHash creation

4fd4b0e

Merge branch 'master' of github.com:ged-lab/khmer into feature/labelg…

6868ccc

…raph

updated ChangeLog for LabelHash changes

481544d

removed unused variable

b412bf5

ctb mentioned this pull request May 27, 2015

Add file type signatures to binary files #1031

Merged

ctb changed the title ~~Some hacking on labelgraph.~~ Some hacking on labelhash. May 27, 2015

mr-c reviewed May 27, 2015
View reviewed changes

khmer/__init__.py

return c

class CountingLabelHash(_LabelHash):

Copy link

Contributor

mr-c May 27, 2015

Choose a reason for hiding this comment

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

This class has no test coverage.

mr-c reviewed May 27, 2015
View reviewed changes

ctb added 2 commits May 27, 2015 12:53

cleanup and more test coverage

99d92ff

added test coverage; removed unused function

e3ff23f

ctb mentioned this pull request May 28, 2015

write truncation tests for file load functions #1037

Closed

clean up memory leak on error exit

f03d3c1

mr-c added a commit that referenced this pull request May 29, 2015

Merge pull request #1021 from dib-lab/feature/labelgraph

819ac87

Some hacking on labelhash.

mr-c merged commit 819ac87 into master May 29, 2015

ctb mentioned this pull request May 29, 2015

Refactor CPython layer so that CountingHash and Hashbits inherit from Hashtable. #1033

Closed

ctb deleted the feature/labelgraph branch June 5, 2015 01:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some hacking on labelhash. #1021

Some hacking on labelhash. #1021

ctb commented May 21, 2015

camillescott commented May 21, 2015

ctb commented May 21, 2015

camillescott commented May 21, 2015

ctb commented May 21, 2015

camillescott commented May 21, 2015

ctb commented May 21, 2015

mr-c May 26, 2015

ctb May 26, 2015

ctb commented May 26, 2015

ctb commented May 26, 2015

mr-c May 27, 2015

mr-c May 27, 2015

mr-c commented May 27, 2015

ctb commented May 28, 2015

ctb commented May 28, 2015

mr-c commented May 28, 2015

ctb commented May 28, 2015

ctb commented May 28, 2015 via email

ctb commented May 28, 2015 via email

ctb commented May 28, 2015

mr-c commented May 28, 2015

Some hacking on labelhash. #1021

Some hacking on labelhash. #1021

Conversation

ctb commented May 21, 2015

camillescott commented May 21, 2015

ctb commented May 21, 2015

camillescott commented May 21, 2015

ctb commented May 21, 2015

camillescott commented May 21, 2015

ctb commented May 21, 2015

mr-c May 26, 2015

Choose a reason for hiding this comment

ctb May 26, 2015

Choose a reason for hiding this comment

ctb commented May 26, 2015

ctb commented May 26, 2015

mr-c May 27, 2015

Choose a reason for hiding this comment

mr-c May 27, 2015

Choose a reason for hiding this comment

mr-c commented May 27, 2015

ctb commented May 28, 2015

ctb commented May 28, 2015

mr-c commented May 28, 2015

ctb commented May 28, 2015

ctb commented May 28, 2015 via email

ctb commented May 28, 2015 via email

ctb commented May 28, 2015

mr-c commented May 28, 2015