remake of #909 w/tests #1067

bocajnotnef · 2015-06-06T20:35:26Z

@ctb moved the pull here as I don't have write access to @drtamermansour's repo.

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?

…ount

Conflicts: scripts/abundance-dist.py

ctb · 2015-06-07T22:06:45Z

Copied over from #909 (putting in comment so as to set up automatic link)

ctb · 2015-06-07T22:29:16Z

OK, took me a while to wrap my head around this.

The complication is that abundance-dist is used with countgraphs created by load-into-counting.

So we end up with the following states:

if load-into-counting BIGCOUNT=no, and abundance-dist BIGCOUNT=yes, BIGCOUNT=no (??)
if load-into-counting BIGCOUNT=no, and abundance-dist BIGCOUNT=no, BIGCOUNT=no (OK)
if load-into-counting BIGCOUNT=yes, and abundance-dist BIGCOUNT=no, BIGCOUNT=no (...OK)
if load-into-counting BIGCOUNT=yes, and abundance-dist BIGCOUNT=yes, BIGCOUNT=no (OK)

I would argue that the first behavior is clearly insane and unexpected, while the third behavior is at least expected if not clearly sane. BUT we are now running into the problem that the default behavior of abundance-dist is to have BIGCOUNT default to "yes", which will trigger the insane case :).

So, @bocajnotnef, could you add a warning message printed out at the beginning (and maybe end?) of the script, for the situation where load-into-counting has produced a countgraph with BIGCOUNT=no? You can use countgraph.get_use_bigcount(), I think. (And add a test for this, too.)

bocajnotnef · 2015-06-07T23:26:39Z

Warning as in "Don't do that, it's bad, sys.exit(1)" or warning as in
"you're doing weird things and I'm not sure how it's gonna end but I'll do
the things anyway"?

On Sun, Jun 7, 2015, 18:29 C. Titus Brown notifications@github.com wrote:

OK, took me a while to wrap my head around this.

The complication is that abundance-dist is used with countgraphs created
by load-into-counting.

So we end up with the following states:

if load-into-counting BIGCOUNT=no, and abundance-dist BIGCOUNT=yes,
BIGCOUNT=no (??)

if load-into-counting BIGCOUNT=no, and abundance-dist BIGCOUNT=no,
BIGCOUNT=no (OK)

if load-into-counting BIGCOUNT=yes, and abundance-dist BIGCOUNT=no,
BIGCOUNT=no (...OK)

if load-into-counting BIGCOUNT=yes, and abundance-dist BIGCOUNT=yes,
BIGCOUNT=no (OK)

I would argue that the first behavior is clearly insane and unexpected,
while the third behavior is at least expected if not clearly sane. BUT we
are now running into the problem that the default behavior of
abundance-dist is to have BIGCOUNT default to "yes", which will trigger the
insane case :).

So, @bocajnotnef https://github.com/bocajnotnef, could you add a
warning message printed out at the beginning (and maybe end?) of the
script, for the situation where load-into-counting has produced a
countgraph with BIGCOUNT=no? You can use countgraph.get_use_bigcount(), I
think. (And add a test for this, too.)

—
Reply to this email directly or view it on GitHub
#1067 (comment).

ctb · 2015-06-07T23:47:15Z

2nd!

Titus Brown, ctbrown@ucdavis.edu

On Jun 7, 2015, at 7:26 PM, Jake Fenton notifications@github.com wrote:

Warning as in "Don't do that, it's bad, sys.exit(1)" or warning as in
"you're doing weird things and I'm not sure how it's gonna end but I'll do
the things anyway"?

On Sun, Jun 7, 2015, 18:29 C. Titus Brown notifications@github.com wrote:

OK, took me a while to wrap my head around this.

The complication is that abundance-dist is used with countgraphs created
by load-into-counting.

So we end up with the following states:

if load-into-counting BIGCOUNT=no, and abundance-dist BIGCOUNT=yes,
BIGCOUNT=no (??)

if load-into-counting BIGCOUNT=no, and abundance-dist BIGCOUNT=no,
BIGCOUNT=no (OK)

if load-into-counting BIGCOUNT=yes, and abundance-dist BIGCOUNT=no,
BIGCOUNT=no (...OK)

if load-into-counting BIGCOUNT=yes, and abundance-dist BIGCOUNT=yes,
BIGCOUNT=no (OK)

I would argue that the first behavior is clearly insane and unexpected,
while the third behavior is at least expected if not clearly sane. BUT we
are now running into the problem that the default behavior of
abundance-dist is to have BIGCOUNT default to "yes", which will trigger the
insane case :).

So, @bocajnotnef https://github.com/bocajnotnef, could you add a
warning message printed out at the beginning (and maybe end?) of the
script, for the situation where load-into-counting has produced a
countgraph with BIGCOUNT=no? You can use countgraph.get_use_bigcount(), I
think. (And add a test for this, too.)

—
Reply to this email directly or view it on GitHub
#1067 (comment).

—
Reply to this email directly or view it on GitHub.

bocajnotnef · 2015-06-08T20:48:06Z

retest this please

bocajnotnef · 2015-06-08T20:52:58Z

@ctb Added the things.

ctb · 2015-06-08T22:50:47Z

Um, how about checking to see if the loaded counting table has bigcount set to no and only warning then?

Also, ChangeLog. And checklist :)

chadwhitacre · 2015-06-10T18:38:23Z

This makes me happy. :)

💯 @ctb @bocajnotnef

ctb · 2015-06-12T10:24:38Z

ping @bocajnotnef - no hurry, but still stuff to do.

bocajnotnef · 2015-06-13T19:21:46Z

@ctb Weird thing: Poking around in abundance-dist shows: counting_hash.set_use_bigcount(args.bigcount)

Does that not override in the way that we expect, and that's the source of the problem?

ctb · 2015-06-13T19:29:18Z

abundance-dist does the reporting, load-into-counting does the counting. You can't report higher than 255 if you haven't counted higher than 255. Happy to give you a test that screws up if you like?

bocajnotnef · 2015-06-13T19:30:30Z

That's alright. Just making sure I had the bits that concern me understood.

On Sat, Jun 13, 2015, 15:29 C. Titus Brown notifications@github.com wrote:

abundance-dist does the reporting, load-into-counting does the counting.
You can't report higher than 255 if you haven't counted higher than 255.
Happy to give you a test that screws up if you like?

—
Reply to this email directly or view it on GitHub
#1067 (comment).

stuff will break

bocajnotnef · 2015-06-13T19:52:37Z

retest this please

Conflicts: scripts/load-into-counting.py

bocajnotnef · 2015-06-13T20:03:43Z

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?

bocajnotnef · 2015-06-13T20:07:08Z

@ctb Did the stuff.

Thing of note: Don't know a good way to add a reminder at the end of the script but the output is so minimal I don't think it matters.

Conflicts: ChangeLog tests/test_scripts.py

ctb · 2015-06-14T12:47:00Z

thx, @bocajnotnef !

remake of #909 w/tests

drtamermansour and others added 5 commits April 4, 2015 16:07

add -b argument to abundance-dist.py

85cc36d

fix pep8 report

0d7a7a2

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

3c2f0bd

…ount

Merge branch 'master' of github.com:ged-lab/khmer into nobigcount/tests

a201f29

Conflicts: scripts/abundance-dist.py

changed test to use the shiny new arg

92b5396

bocajnotnef mentioned this pull request Jun 6, 2015

Add -b to abundance-dist.py #909

Closed

bocajnotnef added 2 commits June 8, 2015 16:42

added warnings, added tests for warnings

0d3731f

pep8

0e43803

bocajnotnef added 2 commits June 13, 2015 15:41

did merge--accidentally did other things in the middle of the merge.

a588de6

stuff will break

pep8, updated tests

69b9385

bocajnotnef added 2 commits June 13, 2015 15:57

Merge branch 'master' of github.com:ged-lab/khmer into nobigcount/tests

f6ef4e8

Conflicts: scripts/load-into-counting.py

added changelog, removed extra line

145ff0b

ctb added 2 commits June 14, 2015 08:39

Merge branch 'master' of github.com:dib-lab/khmer into nobigcount/tests

a085430

Conflicts: ChangeLog tests/test_scripts.py

made error message more explicit

1643d34

ctb added a commit that referenced this pull request Jun 14, 2015

Merge pull request #1067 from dib-lab/nobigcount/tests

2b55caf

remake of #909 w/tests

ctb merged commit 2b55caf into master Jun 14, 2015

ctb deleted the nobigcount/tests branch June 14, 2015 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remake of #909 w/tests #1067

remake of #909 w/tests #1067

bocajnotnef commented Jun 6, 2015

ctb commented Jun 7, 2015

ctb commented Jun 7, 2015

bocajnotnef commented Jun 7, 2015

ctb commented Jun 7, 2015

bocajnotnef commented Jun 8, 2015

bocajnotnef commented Jun 8, 2015

ctb commented Jun 8, 2015

chadwhitacre commented Jun 10, 2015

ctb commented Jun 12, 2015

bocajnotnef commented Jun 13, 2015

ctb commented Jun 13, 2015

bocajnotnef commented Jun 13, 2015

bocajnotnef commented Jun 13, 2015

bocajnotnef commented Jun 13, 2015

bocajnotnef commented Jun 13, 2015

ctb commented Jun 14, 2015

remake of #909 w/tests #1067

remake of #909 w/tests #1067

Conversation

bocajnotnef commented Jun 6, 2015

ctb commented Jun 7, 2015

ctb commented Jun 7, 2015

bocajnotnef commented Jun 7, 2015

ctb commented Jun 7, 2015

bocajnotnef commented Jun 8, 2015

bocajnotnef commented Jun 8, 2015

ctb commented Jun 8, 2015

chadwhitacre commented Jun 10, 2015

ctb commented Jun 12, 2015

bocajnotnef commented Jun 13, 2015

ctb commented Jun 13, 2015

bocajnotnef commented Jun 13, 2015

bocajnotnef commented Jun 13, 2015

bocajnotnef commented Jun 13, 2015

bocajnotnef commented Jun 13, 2015

ctb commented Jun 14, 2015