Make `Read` accessible from python #1492

betatim · 2016-10-26T17:05:15Z

Fixes #769

This makes khmer.Read accessible from python land. If you do not set one of the attributes hasattr will return False.

Is it mergeable?
make test Did it pass the tests?
make clean diff-cover If it introduces new functionality in
scripts/ is it tested?
make format diff_pylint_report cppcheck doc pydocstyle Is it well
formatted?
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?

luizirber · 2016-10-26T19:33:42Z

khmer/_khmer.cc

@@ -308,9 +352,66 @@ static PyGetSetDef khmer_Read_accessors [ ] = {
 };


+static
+PyObject *
+khmer_Read_getattr(khmer_Read_Object *self, PyObject *attr_name)


Is this necessary, since we have a PyGetSetDef defined?
https://github.com/dib-lab/khmer/pull/1492/files#diff-944b784821ddf1048fc73488ee2ee675R329

I will try if raising the excepting in the getter works. Good idea!

You were right :)

codecov-io · 2016-10-27T09:02:52Z

Current coverage is 95.80% (diff: 100%)

No coverage report found for master at 34711f6.

Powered by Codecov. Last update 34711f6...240d970

betatim · 2016-10-27T09:06:44Z

Ready for review! @luizirber, @camillescott, @standage , or @ctb

ctb · 2016-10-27T12:12:29Z

ChangeLog

@@ -1,3 +1,7 @@
+2016-10-27  Tim Head  <betatim@gmail.com>
+
+   * Make khmer's Read type accesible from the python API.


accessible - two ses :)

coughcough fixed

ctb · 2016-10-27T12:13:17Z

khmer/_khmer.cc

+        const_cast<char *>("quality"), const_cast<char *>("annotations"), NULL
+    };
+
+    if (!PyArg_ParseTupleAndKeywords(args, kwds, "|zzzz", kwlist,


Name and sequence shouldn't be optional, I think?

Agreed. Now we have khmer.Read(name, seq) vs screed.Record(dict(name=name, sequence=sequence)). Actually you can pass any kind of dict to Record.

Should we move Read to this "pass a dict as only arg" interface? I prefer the named parameters, but if you frequently turn dicts into Reads it will get tedious typing Read(*my_dict).

betatim · 2016-10-31T08:16:58Z

@luizirber do you have an opinion on the following:

We have khmer.Read(name, seq) vs screed.Record(dict(name=name, sequence=sequence)). Does that difference in interface bother you?

Should we move Read to this "pass a dict as only arg" interface? I prefer the named parameters, but if you frequently turn dicts into Reads it will get tedious typing Read(*my_dict).

ctb · 2016-10-31T12:40:20Z

On Mon, Oct 31, 2016 at 01:16:58AM -0700, Tim Head wrote:

@luizirber do you have an opinion on the following:

We have khmer.Read(name, seq) vs screed.Record(dict(name=name, sequence=sequence)). Does that difference in interface bother you?

screed.Record should be changed to support that IMO.

betatim · 2016-10-31T13:49:22Z

Merge once travis is happy?

ctb · 2016-10-31T13:51:12Z

Agreed.

ctb · 2016-10-31T14:41:29Z

Build failed. Update from latest master, fix build, => merge?

betatim · 2016-11-02T14:15:31Z

This is the error message: https://travis-ci.org/dib-lab/khmer/jobs/171994487#L1841-L1870

betatim · 2016-11-02T14:17:54Z

This failing test is super weird. make clean && make -> test fails, edit test case to use -x 10e7, rerun tests -> passes, undo edit, test still passes. make clean && make and it fails again.

ctb · 2016-11-02T14:19:55Z

could be related to #1248 My suggestion: make it '98 or 99' and commit; we'll deal with thread safety better later.

…

--titus

betatim · 2016-11-02T14:20:16Z

travis is happy right now...so merge?!

ctb · 2016-11-02T14:26:44Z

I would prefer that the uncertainty be addressed explicitly so that we don't face this in every test going forward ;).

betatim · 2016-11-02T14:42:01Z

Running the test in a loop 40 times also surfaced this failure mode

>           assert line == '1,96,96,0.98', line
E           AssertionError: 1,96,96,0.96
E           assert '1,96,96,0.96' == '1,96,96,0.98'
E             - 1,96,96,0.96
E             ?            ^
E             + 1,96,96,0.98
E             ?            ^

ctb · 2016-11-02T15:07:16Z

alas... well, perhaps fix it in #1495?

betatim · 2016-11-02T15:24:26Z

I would make a new PR to fix this. At least I have no good idea where to start looking for the race condition so. Empirically this test doesn't seem to happen that often: "a few times in 100 runs"

There are indeed 98 kmers in the file:

import  tests.khmer_tst_utils as utils
def kmer(seq, k=17):
  for i in range(0, len(seq)-k+1):
    yield seq[i:i+k]

kmers = []
for rec in screed.open(utils.copy_test_data('test-abund-read-2.fa')):
  kmers += list(kmer(rec.sequence))

in case we need this snippet again.

ctb · 2016-11-02T15:43:57Z

On Wed, Nov 02, 2016 at 08:24:26AM -0700, Tim Head wrote:

I would make a new PR to fix this. At least I have no good idea where to start looking for the race condition so. Empirically this test doesn't seem to happen that often: "a few times in 100 runs"

It's clearly in the 'add' function in Hashtable; that's the only place
the relevant things are incremented.

If one of the attributes is not set the instance should behave as if it does not have that attribute.

betatim · 2016-11-07T15:32:52Z

merge?

(edit: no, just scrolled up far enough to realise there is homework left)

ctb · 2016-11-07T15:34:39Z

I'm fine with merging...

ctb · 2016-11-07T15:41:26Z

LGTM - if the thread issue (re)surfaces on CI, let's fix it then.

luizirber requested changes Oct 26, 2016

View reviewed changes

luizirber approved these changes Oct 26, 2016

View reviewed changes

betatim force-pushed the feature/access-Read-type branch 2 times, most recently from 9db6883 to 1822270 Compare October 27, 2016 08:46

betatim force-pushed the feature/access-Read-type branch from 1822270 to fcecdf6 Compare October 27, 2016 09:05

ctb reviewed Oct 27, 2016

View reviewed changes

betatim force-pushed the feature/access-Read-type branch from 0d5c365 to dd795bc Compare October 31, 2016 08:14

betatim mentioned this pull request Oct 31, 2016

Named parameters for screed.Record constructor #1498

Closed

betatim force-pushed the feature/access-Read-type branch from dd795bc to c58a450 Compare October 31, 2016 13:47

betatim force-pushed the feature/access-Read-type branch from c58a450 to aaa8675 Compare October 31, 2016 14:08

betatim mentioned this pull request Nov 2, 2016

test_load_graph_multithread() in test_scripts.py finds different unique k-mers #1248

Open

betatim force-pushed the feature/access-Read-type branch from 94586a0 to ec1c7f9 Compare November 3, 2016 16:12

Make the Read type accessible from python

7ddf8c6

betatim added 7 commits November 4, 2016 15:55

Basic attribute 'hiding'

8feeb78

Test attribute lookup behaviour

d70e3ba

If one of the attributes is not set the instance should behave as if it does not have that attribute.

No need for getattr method

e7581f2

Update test for an empty attribute

7029332

Ehrm, spelling.

c3980b4

Mandatory arguments

c7fb26b

Fix wrong init return value

240d970

betatim force-pushed the feature/access-Read-type branch from ec1c7f9 to 240d970 Compare November 4, 2016 14:57

ctb merged commit 637e04b into dib-lab:master Nov 7, 2016

betatim deleted the feature/access-Read-type branch November 7, 2016 17:51

betatim mentioned this pull request Nov 18, 2016

Fix streaming gzip output #1530

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Read` accessible from python #1492

Make `Read` accessible from python #1492

betatim commented Oct 26, 2016 •

edited

Loading

luizirber Oct 26, 2016

betatim Oct 26, 2016

betatim Oct 26, 2016

codecov-io commented Oct 27, 2016 •

edited

Loading

betatim commented Oct 27, 2016

ctb Oct 27, 2016

betatim Oct 27, 2016

ctb Oct 27, 2016

betatim Oct 28, 2016

betatim commented Oct 31, 2016

ctb commented Oct 31, 2016

betatim commented Oct 31, 2016

ctb commented Oct 31, 2016

ctb commented Oct 31, 2016

betatim commented Nov 2, 2016

betatim commented Nov 2, 2016

ctb commented Nov 2, 2016 via email

betatim commented Nov 2, 2016

ctb commented Nov 2, 2016

betatim commented Nov 2, 2016

ctb commented Nov 2, 2016 via email

betatim commented Nov 2, 2016

ctb commented Nov 2, 2016

betatim commented Nov 7, 2016 •

edited

Loading

ctb commented Nov 7, 2016

ctb commented Nov 7, 2016

		@@ -1,3 +1,7 @@
		2016-10-27 Tim Head <betatim@gmail.com>

		* Make khmer's Read type accesible from the python API.

Make Read accessible from python #1492

Make Read accessible from python #1492

Conversation

betatim commented Oct 26, 2016 • edited Loading

luizirber Oct 26, 2016

Choose a reason for hiding this comment

betatim Oct 26, 2016

Choose a reason for hiding this comment

betatim Oct 26, 2016

Choose a reason for hiding this comment

codecov-io commented Oct 27, 2016 • edited Loading

Current coverage is 95.80% (diff: 100%)

betatim commented Oct 27, 2016

ctb Oct 27, 2016

Choose a reason for hiding this comment

betatim Oct 27, 2016

Choose a reason for hiding this comment

ctb Oct 27, 2016

Choose a reason for hiding this comment

betatim Oct 28, 2016

Choose a reason for hiding this comment

betatim commented Oct 31, 2016

ctb commented Oct 31, 2016

betatim commented Oct 31, 2016

ctb commented Oct 31, 2016

ctb commented Oct 31, 2016

betatim commented Nov 2, 2016

betatim commented Nov 2, 2016

ctb commented Nov 2, 2016 via email

betatim commented Nov 2, 2016

ctb commented Nov 2, 2016

betatim commented Nov 2, 2016

ctb commented Nov 2, 2016 via email

betatim commented Nov 2, 2016

ctb commented Nov 2, 2016

betatim commented Nov 7, 2016 • edited Loading

ctb commented Nov 7, 2016

ctb commented Nov 7, 2016

Make `Read` accessible from python #1492

Make `Read` accessible from python #1492

betatim commented Oct 26, 2016 •

edited

Loading

codecov-io commented Oct 27, 2016 •

edited

Loading

betatim commented Nov 7, 2016 •

edited

Loading