Perl and Python, a practical side-by-side example.

[Shawn Milo]

I'm new to Python and fairly experienced in Perl, although that
experience is limited to the things I use daily.

I wrote the same script in both Perl and Python, and the output is
identical. The run speed is similar (very fast) and the line count is
similar.

Now that they're both working, I was looking at the code and wondering
what Perl-specific and Python-specific improvements to the code would
look like, as judged by others more knowledgeable in the individual
languages.

I am not looking for the smallest number of lines, or anything else
that would make the code more difficult to read in six months. Just
any instances where I'm doing something inefficiently or in a "bad"
way.

I'm attaching both the Perl and Python versions, and I'm open to
comments on either. The script reads a file from standard input and
finds the best record for each unique ID (piid). The best is defined
as follows: The newest expiration date (field 5) for the record with
the state (field 1) which matches the desired state (field 6). If
there is no record matching the desired state, then just take the
newest expiration date.

Thanks for taking the time to look at these.

Shawn

############### ############### ############### ############### ##############
Perl code:
############### ############### ############### ############### ##############
#! /usr/bin/env perl

use warnings;
use strict;

my $piid;
my $row;
my %input;
my $best;
my $curr;

foreach $row (<>){

chomp($row);
$piid = (split(/\t/, $row))[0];

push ( @{$input{$piid} }, $row );
}

for $piid (keys(%input)){

$best = "";

for $curr (@{$input{$piid }}){
if ($best eq ""){
$best = $curr;
}else{
#If the current record is the correct state

if ((split(/\t/, $curr))[1] eq (split(/\t/, $curr))[6]){
#If existing record is the correct state
if ((split(/\t/, $best))[1] eq (split(/\t/, $curr))[6]){
if ((split(/\t/, $curr))[5] gt (split(/\t/, $best))[5]){
$best = $curr;
}
}else{
$best = $curr;
}
}else{
#if the existing record does not have the correct state
#and the new one has a newer expiration date
if (((split(/\t/, $best))[1] ne (split(/\t/, $curr))[6]) and
((split(/\t/, $curr))[5] gt (split(/\t/, $best))[5])){
$best = $curr;
}
}
}
}
print "$best\n";
}

############### ############### ############### ############### ##############
End Perl code
############### ############### ############### ############### ##############


############### ############### ############### ############### ##############
Python code
############### ############### ############### ############### ##############

#! /usr/bin/env python

import sys

input = sys.stdin

recs = {}

for row in input:
row = row.rstrip('\n' )
piid = row.split('\t')[0]
if recs.has_key(pi id) is False:
recs[piid] = []
recs[piid].append(row)

for piid in recs.keys():
best = ""
for current in recs[piid]:
if best == "":
best = current;
else:
#If the current record is the correct state
if current.split(" \t")[1] == current.split(" \t")[6]:
#If the existing record is the correct state
if best.split("\t" )[1] == best.split("\t" )[6]:
#If the new record has a newer exp. date
if current.split(" \t")[5] best.split("\t" )[5]:
best = current
else:
best = current
else:
#If the existing record does not have the correct state
#and the new record has a newer exp. date
if best.split("\t" )[1] != best.split("\t" )[6] and
current.split(" \t")[5] best.split("\t" )[5]:
best = current

print best
############### ############### ############### ############### ##############
End Python code
############### ############### ############### ############### ##############

[bearophileHUGS]

Few suggestions, some important, some less important. All my
suggestions are untested.
Use 4 spaces to indent.
If you want to speed up this code you can move it inside a function.
After that, if you want to make it even faster you can use Psyco too.
Ho are the dates represented? How do you test what is the older one?
You seem to compute current.split(" \t") and best.split("\t" ) many
times, so you can compute it once only.
You can keep best and best_splitted.
You can split the last line:
if best.split("\t" )[1] != best.split("\t" )[6] and \
current.split(" \t")[5] best.split("\t" )[5]:
input() is a builtin function, so you may want to use a different
name, or just:
for row in sys.stdin:
Instead of:
row = row.rstrip('\n' )
You often may use just:
row = row.strip()
Instead of:
piid = row.split('\t')[0]
You can probably use (test it):
piid = row.split('\t', 1)[0]
Instead of:
if recs.has_key(pi id) is False:
Better:
if piid not in recs:
Instead of (on Python 2.5):
recs = {}
for row in input:
row = ...
piid = ...
if recs.has_key(pi id) is False:
recs[piid] = []
recs[piid].append(row)
You can probably use:
from collection import defaultdict
recs = defaultdict(lis t)
for row in input:
row = ...
piid = ...
recs[piid].append(row)
Instead of:
for piid in recs.keys():
You can use this, lazily:
for piid in recs:
Instead of:
for piid in recs.keys():
best = ""
for current in recs[piid]:
You can probably use:
for piid, piii_recs in recs.iteritems( ):
best = ""
for current in piii_recs:
But your version may be a little faster anyway.
Instead of:
best = ""
for current in recs[piid]:
if best == "":
best = current;
You may want to use the singleton None:
best = None
for current in recs[piid]:
if best is None:
best = current
Note that to read such files you may use the csv module (present in
the standard library).

You can try to modify the code and show us the second working version.

Bye,
bearophile

[John Machin]

On Mar 3, 9:44 am, "Shawn Milo" <S...@Milochik. comwrote:

I'm new to Python and fairly experienced in Perl, although that
experience is limited to the things I use daily.

I wrote the same script in both Perl and Python, and the output is
identical. The run speed is similar (very fast) and the line count is
similar.

Now that they're both working, I was looking at the code and wondering
what Perl-specific and Python-specific improvements to the code would
look like, as judged by others more knowledgeable in the individual
languages.

I am not looking for the smallest number of lines, or anything else
that would make the code more difficult to read in six months. Just
any instances where I'm doing something inefficiently or in a "bad"
way.

I'm attaching both the Perl and Python versions, and I'm open to
comments on either. The script reads a file from standard input and
finds the best record for each unique ID (piid). The best is defined
as follows: The newest expiration date (field 5) for the record with
the state (field 1) which matches the desired state (field 6). If
there is no record matching the desired state, then just take the
newest expiration date.

[big snip]
Here is my rewrite in what I regard as idiomatic reasonably-modern
Python (OMMV of course). A few of the comments are applicable
irrespective of the language used.

HTH,
John
8<------
#! /usr/bin/env python

### Layout: Use 4-space indent. Don't use tabs. Don't exceed 79 chars
per line.

import sys

def process_file(op ened_file=sys.s tdin):
### Local variable access is faster than global

### input = sys.stdin ### shadowing built_in function "input"

### Use names to access elements in rows
PIID = 0
ACTUAL_STATE = 1
DESIRED_STATE = 6
EXPIRY_DATE = 5

recs = {}

for row in opened_file:
row = row.rstrip('\n' ).split('\t')
### Do the split('\t') *once* per row
piid = row[PIID]
### if recs.has_key(pi id) is False:
### has_key() is ancient
### "if boolean_express ion is False" is megabletchworth y;
### use "if not boolean_express ion"
if piid not in recs:
recs[piid] = []
recs[piid].append(row)

### for piid in recs.keys():
for piid in recs:
best = None ### use out-of-band sentinel
for current in recs[piid]:
if best is None:
best = current ### had cockroach crap (";") at EOL
else:
#If the current record is the correct state
### Clear code (like the next line) doesn't need
comments
### like the above line
if current[ACTUAL_STATE] == current[DESIRED_STATE]:
if best[ACTUAL_STATE] == best[DESIRED_STATE]:
if current[EXPIRY_DATE] best[EXPIRY_DATE]:
best = current
else:
best = current
else:
if (best[ACTUAL_STATE] != best[ACTUAL_STATE]
and current[EXPIRY_DATE] best[EXPIRY_DATE]):
best = current
print "\t".join(b est)

if __name__ == "__main__":
### Standard idiom to avoid executing script content
### when/if file is imported
process_file()
8<----

[Bruno Desthuilliers]

Shawn Milo a écrit :

I'm new to Python and fairly experienced in Perl, although that
experience is limited to the things I use daily.

I wrote the same script in both Perl and Python, and the output is
identical. The run speed is similar (very fast) and the line count is
similar.

Now that they're both working, I was looking at the code and wondering
what Perl-specific and Python-specific improvements to the code would
look like, as judged by others more knowledgeable in the individual
languages.

I am not looking for the smallest number of lines, or anything else
that would make the code more difficult to read in six months. Just
any instances where I'm doing something inefficiently or in a "bad"
way.

(snip)

>
#! /usr/bin/env python

import sys

input = sys.stdin

recs = {}

for row in input:
row = row.rstrip('\n' )
piid = row.split('\t')[0]
if recs.has_key(pi id) is False:

'is' is the identity operator - practically, in CPython, it compares
memory addresses. You *dont* want to use it here.

Another way to test if a key is already in a dict is to use the 'in'
operator, ie:
if not piid in recs:

recs[piid] = []
recs[piid].append(row)

As a last point, dicts have a setdefault(key, default) method that
returns the value associated with key if it already exists, else add the
key=>default pair and returns default, but it's a little bit slower.
for row in input:
row = row.rstrip('\n' )
piid = row.split('\t')[0]
recs.setdefault (piid, []).append(row)

or

for row in input:
row = row.rstrip('\n' )
piid = row.split('\t')[0]
if piid not in recs:
recs[piid] = []
recs[piid].append(row)

for piid in recs.keys():

You don't need to call key() here - it's already the default iteration
for dicts.

But since you want the values too, you should use dict.items() or
dict.iteritems( ):

for piid, rows in recs.iteritems( )

best = ""
for current in recs[piid]:
if best == "":
best = current;

Get rid of this ";" !-)

Better to use None - it's the "no value" object, and it let you use an
identity test:

best = None
for current in rows:
if best is None:
best = row

And while we're at it, you can save yourself a test here:

best = row[0]
for current in row[1:]:
# start tests

#If the current record is the correct state
if current.split(" \t")[1] == current.split(" \t")[6]:
#If the existing record is the correct state
if best.split("\t" )[1] == best.split("\t" )[6]:
#If the new record has a newer exp. date
if current.split(" \t")[5] best.split("\t" )[5]:

You're repeatingly calling split() on the same objects. This is a
serious waste of time and CPU.

best = current
else:
best = current
else:
#If the existing record does not have the correct state
#and the new record has a newer exp. date
if best.split("\t" )[1] != best.split("\t" )[6] and
current.split(" \t")[5] best.split("\t" )[5]:
best = current

print best

Here's a somewhat corrected version:
# ---
import sys

def findbests(input =sys.stdin, output=sys.stdo ut):
DATE = 5
TARGET = 6
STATE = 1
recs = {}

for row in input:
row = row.rstrip('\n' ).split("\t")
piid = row[0]
if piid not in recs:
recs[piid] = []
recs[piid].append(row)

for piid, rows in recs.iteritems( ):
best = rows[0]
for current in rows[1:]:
if current[STATE] == current[TARGET]:
if best[STATE] == best[TARGET]:
if current[DATE] best[DATE]:
best = current
else:
best = current
elif best[STATE] != best[TARGET] \
and current[DATE] best[DATE]:
best = current

print >output, "\t".join(b est)

if __name__ == '__main__':
findbdest()

# ---

It's somewhat shorter, a bit more readable IMHO, and somewhat faster too
(=~ 30/40% faster on my machine). Also, it's usable as both a program
and an importable module, and with any line input and file-like output.

Now for the bad news: I'm afraid your algorithm is broken : here are my
test data and results:

input = [
#ID STATE ... ... ... TARG DATE
"aaa\tAAA\t...\ t...\t...\tBBB\ t20071212\n",
"aaa\tAAA\t...\ t...\t...\tAAA\ t20070120\n",
"aaa\tAAA\t...\ t...\t...\tAAA\ t20070101\n",
"aaa\tAAA\t...\ t...\t...\tBBB\ t20071010\n",
"aaa\tAAA\t...\ t...\t...\tBBB\ t20071111\n",
"ccc\tAAA\t...\ t...\t...\tBBB\ t20071201\n",
"ccc\tAAA\t...\ t...\t...\tAAA\ t20070101\n",
"ccc\tAAA\t...\ t...\t...\tBBB\ t20071212\n",
"ccc\tAAA\t...\ t...\t...\tAAA\ t20071212\n", # oops !
"bbb\tAAA\t...\ t...\t...\tAAA\ t20070101\n",
"bbb\tAAA\t...\ t...\t...\tAAA\ t20070101\n",
"bbb\tAAA\t...\ t...\t...\tAAA\ t20071212\n",
"bbb\tAAA\t...\ t...\t...\tAAA\ t20070612\n",
"bbb\tAAA\t...\ t...\t...\tBBB\ t20071212\n",
]

=>
aaa AAA ... ... ... BBB 20071212
bbb AAA ... ... ... BBB 20071212
ccc AAA ... ... ... BBB 20071201

At least for piid 'ccc', there's a record with matching state and newest
date. (exact same result as with your original version).

HTH

[Bruno Desthuilliers]

John Machin a écrit :

On Mar 3, 9:44 am, "Shawn Milo" <S...@Milochik. comwrote:

(snip)

>
[big snip]
Here is my rewrite in what I regard as idiomatic reasonably-modern
Python (OMMV of course).

(snip)

John, I *swear* I didn't read your code before posting my own version !

[Paul Rubin]

Here's my version (not tested much). Main differences from yours:

1. It defines a Python class to hold row data, and defines the __cmp__ operation
on the class, so given two Row objects r1,r2, you can say simply
if r1 r2: ...
to see which is "better".

2. Instead of reading all the rows into memory and then scanning the
list of records of each piid, it simply remembers the best it has seen
for each piid.

By putting the "better than" logic into the class definition, the main
loop becomes very simple. It does parse out and store fields on the
Row objects consuming some extra memory, but you could eliminate that
at the cost of a little code and speed by re-parsing as needed in the
comparison function.

=============== =============== =============== =============== ====

#! /usr/bin/env python

import sys

class Row:
def __init__(self, row):
self.row = row.rstrip('\n' )
fields = self.row.split( '\t')
self.piid = fields[0]
self.state = fields[1]
self.expiration _date = fields[5]
self.desired_st ate = fields[6]

def __cmp__(self, other):
# return +1 if self is better than other, -1 if other is better
# than self, or 0 if they are equally good
if self.state == self.desired_st ate:
if other.state != other.desired_s tate:
return 1
return cmp(self.expira tion_date, other.expiratio n_date)
elif other.expiratio n_date self.expiration _date:
# other record is better only if its exp date is newer
return 1
return 0

best = {}
input = sys.stdin

for row in input:
r = Row(row)
if r.piid not in best or r best[r.piid]:
best[r.piid] = r

for piid,r in best.iteritems( ):
print r.row

[John Machin]

On Mar 3, 12:36 pm, Bruno Desthuilliers >
[snip]

DATE = 5
TARGET = 6

[snip]

Now for the bad news: I'm afraid your algorithm is broken : here are my
test data and results:

input = [
#ID STATE ... ... ... TARG DATE
"aaa\tAAA\t...\ t...\t...\tBBB\ t20071212\n",

[snip]

Bruno, The worse news is that your test data is broken. According to
the OP (and your own code (DATE = 5 etc)), the target state comes
last. GIGO. At least you have demonstrated to the OP why naming the
field indexes is a good idea :-)
Cheers,
John

[Bjoern Schliessmann]

Bruno Desthuilliers wrote:

Shawn Milo a écrit :

> if recs.has_key(pi id) is False:

'is' is the identity operator - practically, in CPython, it
compares memory addresses. You *dont* want to use it here.

It's recommended to use "is None"; why not "is False"? Are there
multiple False instances or is False generated somehow?

Regards,
Björn

--
BOFH excuse #135:

You put the disk in upside down.

[Ben Finney]

Bjoern Schliessmann <us************ **************@ spamgourmet.com writes:

Bruno Desthuilliers wrote:

Shawn Milo a écrit :

if recs.has_key(pi id) is False:

'is' is the identity operator - practically, in CPython, it
compares memory addresses. You *dont* want to use it here.

It's recommended to use "is None"; why not "is False"? Are there
multiple False instances or is False generated somehow?

I'd recommend against using "is False" simply because it's more
confusing. This is better::

if not recs.has_key(pi id): # [1]

Moreover, any 'if' statement expects a boolean expression *anyway*, so
there's no point testing for identity against False. Otherwise, the
following would be just as reasonable::

if (recs.has_key(p iid) is False) is True:

Or perhaps:

if (((((recs.has_k ey(piid) is False) is True) is False) is False) is True):
[1]: yes, this is even better written as 'if not piid in recs', but
that's beside the point for this discussion.

--
\ "To be is to do" -- Plato |
`\ "To do is to be" -- Aristotle |
_o__) "Do be do be do" -- Sinatra |
Ben Finney

[attn.steven.kuo]

On Mar 2, 2:44 pm, "Shawn Milo" <S...@Milochik. comwrote:

(snipped)

I'm attaching both the Perl and Python versions, and I'm open to
comments on either. The script reads a file from standard input and
finds the best record for each unique ID (piid). The best is defined
as follows: The newest expiration date (field 5) for the record with
the state (field 1) which matches the desired state (field 6). If
there is no record matching the desired state, then just take the
newest expiration date.

Thanks for taking the time to look at these.

My attempts:

### Perl ###

#!/usr/bin/perl
use strict;
use warnings;

use List::Util qw/reduce/;
use constant {
STATE =1,
DATE =6,
TARGET =5,
};

sub keep_best {
my ($best, $current) = @_;
if ($current->[STATE] eq $current->[TARGET]) {
if ($best->[STATE] eq $best->[TARGET]) {
if ($current->[DATE] gt $best->[DATE]) {
return 0;
}
} else {
return 0;
}
} elsif (
$best->[STATE] ne $best->[TARGET]
and
$current->[DATE] gt $best->[DATE]) {
return 0;
}
return 1;
}
my %input;

# while uses less memory than for:
# the former is an iterator

while (<>)
{
chomp;
my @results = split(/\t/, $_);
my $key = $results[0];
push @{$input{$key}} , [ @results, $_ ];
}

# while uses less memory than for:
# the former is an iterator

while (my ($key, $aref ) = each %input)
{
my $best = reduce {
keep_best( $a, $b ) ? $a : $b
} @$aref;

print $best->[-1], "\n";
}
### Python (re-working John's code) ###

import sys

def keep_best(best, current):

ACTUAL_STATE = 1
# John had these swapped
DESIRED_STATE = 5
EXPIRY_DATE = 6

keep = True
if current[ACTUAL_STATE] == current[DESIRED_STATE]:
if best[ACTUAL_STATE] == best[DESIRED_STATE]:
if current[EXPIRY_DATE] best[EXPIRY_DATE]:
keep = False
else:
keep = False
else:
if (best[ACTUAL_STATE] != best[ACTUAL_STATE]
and current[EXPIRY_DATE] best[EXPIRY_DATE]):
keep = False
return keep

def process_file(op ened_file=sys.s tdin):

PIID = 0
recs = {}

for line in opened_file:
line = line.rstrip('\n ')
row = line.split('\t' )
row.append(line )
piid = row[PIID]
if piid not in recs:
recs[piid] = []
recs[piid].append(row)

for piid in recs:
best = reduce(lambda b, c: keep_best(b, c) and b or c,
recs[piid])
print best[-1]

if __name__ == "__main__":
process_file()
# "reduce" seems to be Lispish, Pythonic, and Perlish!

--
Hope this helps,
Steve