help with making my code more efficient
d at davea.name
Fri Dec 21 04:31:18 CET 2012
On 12/20/2012 08:46 PM, Larry.Martell at gmail.com wrote:
> On Thursday, December 20, 2012 6:17:04 PM UTC-7, Dave Angel wrote:
> Of course it's a fragment - it's part of a large program and I was just showing the relevant parts.
But it seems these are methods in a class, or something, so we're
missing context. And you use self without it being an argument to the
function. Like it's a global.
> Yes, the code works. I end up with just the rows I want.
>> Are you only concerned about speed, not fixing features?
> Don't know what you mean by 'fixing features'. The code does what I want, it just takes too long.
>> As far as I can tell, the logic that includes the time comparison is bogus.
> Not at all.
>> You don't do anything there to worry about the value of tup, just whether some
>> item has a nearby time. Of course, I could misunderstand the spec.
> The data comes from a database. tup is a datetime column. tdiff comes from a datetime.timedelta()
I thought that tup was the datetime. In any case, the loop makes no
sense to me, so I can't really optimize it, just make suggestions.
>> Are you making a global called 'self' ? That name is by convention only
>> used in methods to designate the instance object. What's the attribute
> Yes, self is my instance object. self.message contains the string of interest that I need to look for.
>> Can cdata have duplicates, and are they significant?
> No, it will not have duplicates.
>> Is the list sorted in any way?
> Yes, the list is sorted by tool and datetime.
>> Chances are your performance bottleneck is the doubly-nested loop. You
>> have a list comprehension at top-level code, and inside it calls a
>> function that also loops over the 600,000 items. So the inner loop gets
>> executed 360 billion times. You can cut this down drastically by some
>> judicious sorting, as well as by having a map of lists, where the map is
>> keyed by the tool.
> Thanks. I will try that.
So in your first loop, you could simply split the list into separate
lists, one per tup value, and the lists as dictionary items, keyed by
that tool string.
Then inside the determine() function, make a local ref to the particular
list for the tool.
recs = messageTimes[tup]
Instead of a for loop over recs, use a binary search to identify the
first item that's >= date_time-tdiff. Then if it's less than
date_time+tdiff, return True, otherwise False. Check out the bisect
module. Function bisect_left() should do what you want in a sorted list.
>>> cdata[:] = [tup for tup in cdata if determine(tup)]
>> As the code exists, there's no need to copy the list. Just do a simple
> This statement is to remove the items from cdata that I don't want. I don't know what you mean by bind. I'm not familiar with that python function.
Every "assignment" to a simple name is really a rebinding of that name.
cdata = [tup for tup in cdata if determine(tup)]
will rebind the name to the new object, much quicker than copying. If
this is indeed a top-level line, it should be equivalent. But if in
fact this is inside some other function, it may violate some other
assumptions. In particular, if there are other names for the same
object, then you're probably stuck with modifying it in place, using
BTW, a set is generally much more memory efficient than a dict, when you
don't use the "value". But since I think you'll be better off with a
dict of lists, it's a moot point.
More information about the Python-list