-
Notifications
You must be signed in to change notification settings - Fork 294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleaning up automatic argument setting #1214
Conversation
@@ -112,14 112,13 @@ def optimal_args_output_gen(unique_kmers, fp_rate): | |||
return "\n".join(to_print) | |||
|
|||
|
|||
def do_sanity_checking(args, desired_max_fp): | |||
def check_fp_rate(args, desired_max_fp): | |||
""" | |||
simple function to check if the restrictions in the args (if there are any) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(please also update this - it's not simple anymore :) and also is currently limited to looking at unique_kmers.)
Also, you can deindent most of the function by changing that first 'if' to
if not args.unique_kmers:
return
I like this code so far; nice work. I think the next step should be to encapsulate the call to oxli.functions in khmer.khmer_args create* functions, with a default fp_rate of 0.10. That should then implement it for all the scripts without further modifications, yah? Can you also add a long argument --fp-rate (no short version) that lets people override script-configured fp-rate from the command line? That should let us do:
as a way of setting memory parameters. Make sense? |
Yup, sounds good. I'll get that rolling. On Fri, Jul 31, 2015, 06:47 C. Titus Brown [email protected] wrote:
|
Be sure to remove this: https://github.com/dib-lab/khmer/blob/master/scripts/normalize-by-median.py#L321 and make that a general warning in khmer_args.py. |
Should we sanity check for things like ending up with a negative amount of reccomended memory? Somehow, this then resulted in an FP rate of 2.08. |
What's strange is if I set desired fp rate to be 0.004 or 0.00004 the estimated FP rate comes out to be 0.208 |
well, the equations are approximations. so for small numbers of k-mers weird I would say, if -x is going to be less than 1 mb in size, increase it to On Mon, Aug 03, 2015 at 10:38:37AM -0700, Jake Fenton wrote:
|
Currently, we check to see if we have I vote for doing the check in the function and have just all scripts do |
On Mon, Aug 03, 2015 at 02:00:07PM -0700, Jake Fenton wrote:
Why do you vote that? (I agree, just curious as to your reasoning.) |
Reduces points of failure to a single place--less repeated code in the scripts. If we're gonna do the same check in fifteen places to run a thing, just have the thing do the check. |
On Mon, Aug 03, 2015 at 02:07:20PM -0700, Jake Fenton wrote:
1. Let's see how it works in practice :) |
@ctb Updated--Should I further expand to other htable-using scripts? |
BTW, please make --unique-kmers a float - that lets us use scientific notation to set it, e.g. |
make sense--If not, complain. If no restrictions are given, add some that | ||
make sense. | ||
Takes in args and desired max FP rate | ||
function to check if the desired_max_fp rate makes sense given specified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalize 'function'
Please see #1214 (comment); was there a reason you didn't go with that for normalize-by-median? |
Oh, nuts. Nope, no reason. I just had that conversation with Michael, too. I'll pull the function call out to the create* functions. |
Also, should I be making use of the logging framework we added? 'cause with the addition of the autoarg stuff we break normalize being quiet. |
(which also reminds me, the tests for normalize being quiet don't actually assert that there be nothing in the output, just that there aren't certian things in the output) |
On Tue, Aug 04, 2015 at 11:28:46AM -0700, Jake Fenton wrote:
No, I fixed that. |
On Tue, Aug 04, 2015 at 11:28:23AM -0700, Jake Fenton wrote:
Please fix -q on normalize-by-median, but don't institute it in other |
I'm running into circular import issues between khmer_args and oxli/init. Apparently this is indicative of needing to refactor package structure--Which I'm inclined to agree with. So, unless there's mass objection, I'm gonna migrate the autoarg stuff out of oxli and into khmer_args |
On Tue, Aug 04, 2015 at 11:54:26AM -0700, Jake Fenton wrote:
OK short term fix. |
long term punt to issue? |
On Tue, Aug 04, 2015 at 11:55:53AM -0700, Jake Fenton wrote:
yep. |
(but it will be obvious, no need to punt to an issue; part of oxli
refactoring)
|
Alright. Cool. |
Problem: if we call the check_fp_rate function from the funcs that create hashtables we won't be able to warn people if they're loading a hashtable AND using the automatic args (since we won't ever create a hashtable if we're loading one) Suggested solution: we make the --load-table arg and the -U arg (and/or the --max-mem arg) mutually exclusive in argparse. |
Sounds like a plan.
|
new problem: the args I need to make mutually exclusive are added in two separate places. I'm gonna eat and ponder |
@ctb That was less insane than I was expecting.
Ready for merge |
@@ -110,6 272,16 @@ def __call__(self, parser, namespace, values, option_string=None): | |||
** Your values for ksize, n_tables, and tablesize | |||
** will be ignored.'''.format(hashfile=values)) | |||
|
|||
if getattr(namespace, 'unique_kmers') != 0 or \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason not to use 'namespace.unique_kmers' and 'namespace.max_memory_usage' here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just using the syntax I saw elsewhere. Referencing the attributes directly should be safe here.
@ctb Updated |
@@ -110,6 272,15 @@ def __call__(self, parser, namespace, values, option_string=None): | |||
** Your values for ksize, n_tables, and tablesize | |||
** will be ignored.'''.format(hashfile=values)) | |||
|
|||
if namespace.unique_kmers != 0 or namespace.max_memory_usage: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eliminate != 0
This looks pretty nice and clean to me, but I may not have a chance to dig into it today. My main comment is that I think the script-level testing should be expanded a bit - think about it and let me know what you decide :). |
I'm running into a lot of problems with having things like the LoadAction calls--or any function that gets called when there's an arg. It introduces too many cases to keep track of (for such things as handling args that shouldn't be set together or things like handling--quiet, especially since we can only configure the logger once we parse the args but when we parse the args we spew output everywhere). Working around having these actions is beginning to get non-trivial, so I'm going to factor it all out into a check_conflicting_args function that I'm gonna have something or another call (maybe report_on_config...?) |
(something tells me doing this refactor will be non-trivial regardless) |
Realizing this is probably what I was supposed to do regardless. Oh well. |
retest this, please |
@@ -78,7 93,7 @@ def test_normalize_by_median_quiet(): | |||
shutil.copyfile(utils.get_test_data('test-abund-read-2.fa'), infile) | |||
|
|||
script = 'normalize-by-median.py' | |||
args = ['-C', CUTOFF, '-k', '17', '--quiet', infile] | |||
args = ['-C', CUTOFF, '-k', '17', '--quiet', '-M', '1e6', infile] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I made it so report_on_config executed again I had to increase the tablesize to stop it from warning during the quiet test. Normally, I'd then go "But it shouldn't make any noise when silenced anwyay" but these are warnings, which as I recall we wanted to ignore --quiet.
c8c86ae
to
00389b5
Compare
retest this, please |
@mr-c Ready for merge |
Misc review thoughts
Train of thought -- normalize-by-median code removal is good! |
(I will do these things) |
Nicely done; I like the cleaned up loadhash-checking code, in particular. |
Cleaning up automatic argument setting
Thanks! |
Ref #1117 for expansion of automatic memory usage determination, this time based on estimated number of unique k-mers. Inspired by user experience upgrade goals (#732) and hopefully will fix #1179 (dealing with
-U/--unique-kmers
).This resolves @ctb's comment #1117 (comment):
My response to the last:
Next steps TBD