Vulture of #621: "add estimate_optimal_hash.py" by SensibleSalmon · Pull Request #1106 · dib-lab/khmer

SensibleSalmon · 2015-06-19T16:36:09Z

Copied from #621, addresses #390(ish)

SensibleSalmon · 2015-06-19T16:45:57Z

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?

SensibleSalmon · 2015-06-19T16:50:20Z

retest this, please

SensibleSalmon · 2015-06-19T16:57:44Z

@ctb @luizirber CR/merge, please?

luizirber · 2015-06-19T17:06:35Z

Suggestions:

Move estimate_* functions to oxli/functions.py
Merge optimal_args_HLL.py into unique-kmers.py (both in sandbox)

luizirber · 2015-06-19T18:23:43Z

Can we use print("{0}".format(err)) here? (source)

SensibleSalmon · 2015-06-19T21:16:41Z

@ctb What unit of memory is -M in? Gigs? Megs?

ctb · 2015-06-19T21:22:51Z

Bytes.

Titus Brown, ctbrown@ucdavis.edu

On Jun 19, 2015, at 2:16 PM, Jake Fenton notifications@github.com wrote:

@ctb What unit of memory is -M in? Gigs? Megs?

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

SensibleSalmon · 2015-06-22T17:19:19Z

retest this, please

SensibleSalmon · 2015-06-22T19:00:22Z

retest this please

SensibleSalmon · 2015-06-22T19:17:42Z

@ctb @luizirber @camillescott CR/merge, please?

ctb · 2015-06-22T20:38:18Z

(Please wait to ask for a CR or a merge until after the tests pass - thanks :)

SensibleSalmon · 2015-06-22T23:26:08Z

Weird. 2.7 and 3.3 had passed last I saw.

Sigh.

On Mon, Jun 22, 2015, 16:38 C. Titus Brown notifications@github.com wrote:

(Please wait to ask for a CR or a merge until after the tests pass -
thanks :)

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

luizirber · 2015-06-22T23:31:03Z

this can be imported from oxli.functions now.

SensibleSalmon · 2015-06-23T01:56:12Z

retest this please

SensibleSalmon · 2015-06-23T03:51:29Z

@ctb @luizirber @camillescott CR and merge, please

Also, for next steps: I move that the argparser stuff currently here https://github.com/dib-lab/khmer/pull/1106/files#diff-c9f39ec665e36b5911296d1f4028270cR61 gets moved into the oxli module so it can be added to scripts easily (a la threading args). Some additional processing stuff will need to be added to the scripts that implement this but it should be along the lines of "if memory options then use returned values"

ctb · 2015-06-23T13:26:27Z

shouldn't this be -N?

ping @bocajnotnef

ctb · 2015-06-23T13:28:26Z

Regarding your "next steps", above, could you (1) read through #1050 and (2) create a new issue with your proposal, taking into account the changes in #1050?

ctb · 2015-06-23T13:29:09Z

please document what N and f are, thx :)

SensibleSalmon · 2015-06-24T18:13:35Z

@ctb @luizirber CR/merge, please?

luizirber · 2015-06-24T18:55:26Z

Ahem, sorry. This output is fast to calculate, and it would be useful on sandbox/unique-kmers.py too. Which is basically this script, minus this output. Merge them, please? =]

Ahem, sorry. This output is fast to calculate, and it would be useful on `sandbox/unique-kmers.py` too. Which is basically this script, minus this output. Merge them, please? =]

Didn't you make this suggestion in #1106 (comment), too?

Huh. Maybe he didn't see it.

Moving lines.

Woo reading.

SensibleSalmon · 2015-06-24T19:31:53Z

retest this please

SensibleSalmon · 2015-06-24T19:51:40Z

retest this please

SensibleSalmon · 2015-06-24T20:38:22Z

Retest this please.

SensibleSalmon · 2015-06-25T00:45:26Z

retest this please

SensibleSalmon · 2015-06-26T17:30:18Z

@ctb @luizirber CR/merge please? Tests/make-diff/pep8 passing locally.

ctb · 2015-06-26T18:06:31Z

On Fri, Jun 26, 2015 at 10:30:18AM -0700, Jake Fenton wrote:

@ctb @luizirber CR/merge please? Tests/make-diff/pep8 passing locally.

I think this belongs in @luizirber's hands :)

Note that if you want to start working on a new PR that's predicated on
this one (or perhaps on two PRs) you can just merge those branches into
a new branch and set up a PR against that one.

luizirber · 2015-06-26T19:58:02Z

this is broken in the output. Shows up as

number of unique k-mers: false positive rate: 108047.000

should be

number of unique k-mers: 108047 false positive rate: 0.01

(or something similar, I just ran over a file I had lying around here).

… tables

… use things that don't exist

…truction method

luizirber · 2015-06-28T15:51:18Z

LGTM, I fixed the ChangeLog.

Vulture of #621: "add estimate_optimal_hash.py"

luizirber reviewed Jun 19, 2015
View reviewed changes

Comment thread tests/test_sandbox_scripts.py Outdated

luizirber Jun 19, 2015

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

Can we use print("{0}".format(err)) here? (source)

luizirber reviewed Jun 22, 2015
View reviewed changes

Comment thread sandbox/optimal_args_HLL.py Outdated

luizirber Jun 22, 2015

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

this can be imported from oxli.functions now.

ctb reviewed Jun 23, 2015
View reviewed changes

Comment thread oxli/functions.py Outdated

ctb Jun 23, 2015

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

please document what N and f are, thx :)

SensibleSalmon mentioned this pull request Jun 23, 2015

Implementing --max-mem and auto'd hashtable agrs #1117

Closed

luizirber reviewed Jun 24, 2015
View reviewed changes

SensibleSalmon closed this Jun 24, 2015

SensibleSalmon reopened this Jun 24, 2015

luizirber reviewed Jun 26, 2015
View reviewed changes

Qingpeng Zhang and others added 15 commits June 28, 2015 15:50

add estimate_optimal_hash.py

6332c69

add optimal_args_hashbits(HLL) to estimate optimal arguments for hash…

aff3222

… tables

cleanup, make it all pretty so it'll pass tests

c81be67

added QP to changelog

efeed1c

copyright

5c5841c

moved functions into oxli/functions; added tests, pep8

4198bde

.format()

a5b66dc

some moar things to import

2405c3a

cleanup, moved things into oxli/functions

63245c3

I can't read

3e541a7

still can't read

3e25dc6

added namedtuple support and tests

621fc4f

added test coverage for oxlified function

ac51a4f

names are better than numbers, spelling is important and we shouldn't…

5302934

… use things that don't exist

updated changelog, removed bad check_space line, switched output cons…

8945ffc

…truction method

luizirber force-pushed the fix/390 branch from 442f874 to 8945ffc Compare June 28, 2015 15:51

luizirber added a commit that referenced this pull request Jun 28, 2015

Merge pull request #1106 from dib-lab/fix/390

fdd8f7a

Vulture of #621: "add estimate_optimal_hash.py"

luizirber merged commit fdd8f7a into master Jun 28, 2015

luizirber deleted the fix/390 branch June 28, 2015 15:53

This was referenced Jun 28, 2015

add estimate_optimal_hash.py #621

Closed

Add implementation of optimal hash size calculation to khmer.khmer_args #390

Closed

This was referenced Jun 29, 2015

Implement auto'd optimal hashtable args and memory limitation checking #1126

Merged

addresses #1141, unique-kmers.py being too noisy, bad naming paradigm for estimate functions #1145

Merged

hashtable arg estimation functions may be wrong #1146

Closed

ctb mentioned this pull request Oct 4, 2016

Choosing hash table size and numbers according to number of unique k-mers #347

Closed

Conversation

SensibleSalmon commented Jun 19, 2015

Uh oh!

SensibleSalmon commented Jun 19, 2015

Uh oh!

SensibleSalmon commented Jun 19, 2015

Uh oh!

SensibleSalmon commented Jun 19, 2015

Uh oh!

luizirber commented Jun 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SensibleSalmon commented Jun 19, 2015

Uh oh!

ctb commented Jun 19, 2015

Uh oh!

SensibleSalmon commented Jun 22, 2015

Uh oh!

SensibleSalmon commented Jun 22, 2015

Uh oh!

SensibleSalmon commented Jun 22, 2015

Uh oh!

ctb commented Jun 22, 2015

Uh oh!

SensibleSalmon commented Jun 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SensibleSalmon commented Jun 23, 2015

Uh oh!

SensibleSalmon commented Jun 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctb commented Jun 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SensibleSalmon commented Jun 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctb Jun 24, 2015 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctb Jun 24, 2015 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SensibleSalmon commented Jun 24, 2015

Uh oh!

SensibleSalmon commented Jun 24, 2015

Uh oh!

SensibleSalmon commented Jun 24, 2015

Uh oh!

SensibleSalmon commented Jun 25, 2015

Uh oh!

SensibleSalmon commented Jun 26, 2015

Uh oh!

ctb commented Jun 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luizirber commented Jun 28, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants