seeking to improve Python skills
mk
I wrote the following program mainly for educational purpose of
improving my Python programming skills -- would Pythonistas here please
look at it and point out areas that could use improvement?
This thing analyzes the stored load average record file, calculates
simple moving average and produces Gnuplot program to plot the thing.
Constructive criticism is welcome.
#!/usr/bin/python
import re
import sys
import tempfile
import subprocess
import os
class MovingAvg(object):
def __init__(self, fname):
try:
self.fo = open(fname)
except IOError, e:
print "Problem with opening file:", e
sys.exit(4)
self.reslist = []
self.smalist = []
self.maxval = 0
def extrfromfile(self):
vre = re.compile("(\d+-\d+-\d+) (\d\d:\d\d) (\d+\.\d+)")
for line in self.fo:
res = vre.search(line)
if res:
self.reslist.append({'day':res.group(1),\
'time':res.group(2),\
'val':float(res.group(3))})
def calc_sma(self, smalen=4):
if smalen == 0:
raise AssertionError, "Moving Average sample
length cannot be 0"
if not isinstance(smalen, int):
raise AssertionError, "Moving Average sample
length has to be integer"
total = 0
total = sum( [ x['val'] for x in self.reslist[0:smalen] ] )
sma = total / smalen
smaidx = int(smalen/2)
self.smalist.append((self.reslist[0]['day'],\
self.reslist[0]['time'],\
self.reslist[0]['val'],\
self.reslist[0]['val']))
for i in range(smalen, len(self.reslist)):
curval = self.reslist[i]['val']
self.maxval = max(self.maxval, curval)
total += curval
total -= self.reslist[i - smalen]['val']
sma = total / smalen
smaidx += 1
self.smalist.append((self.reslist[smaidx]['day'],\
self.reslist[smaidx]['time'],\
self.reslist[smaidx]['val'],\
sma))
def return_results(self):
return (self.reslist, self.smalist, self.maxval)
class GnuplotWrapper(object):
def __init__(self, smalist, maxval, outfname = "calc.png",
graphlen=640, graphheight=480, gnuplot = '/usr/bin/gnuplot'):
self.outfname = outfname
self.smalist = smalist
self.gnuplot = gnuplot
self.gprog = None
self.gdata = None
self.graphlen = graphlen
self.graphheight = graphheight
def _writefiles(self):
self.gprog = tempfile.mkstemp()
self.gdata = tempfile.mkstemp()
self.gprogfile = open(self.gprog[1], 'wb')
self.gdatafile = open(self.gdata[1], 'wb')
labelnum = int(self.graphlen / 110)
labelstep = int(len(self.smalist) / labelnum)
labels = []
for i in range(0, len(self.smalist), labelstep):
labels.append("\"%s %s\" %d" %
(self.smalist[i][0], self.smalist[i][1], i))
labelstr = ", ".join(labels)
self.gprogfile.write("""set terminal png size %d, %d
set style line 1 lt 1 lw 2
set style line 2 lt 2 lw 2
set output "%s"
set xtics(%s)
set yrange [0:%f]
set y2range [0:%f]
plot "%s" using 1 with lines ls 1 title "orig" axes x1y1, "%s" using 2
with lines ls 2 title "Moving Average" axes x1y2
""" % (self.graphlen, self.graphheight, self.outfname, labelstr,
float(maxval), float(maxval),\
self.gdata[1], self.gdata[1]) )
self.gprogfile.close()
for elem in self.smalist:
self.gdatafile.write("%f, %f\n" % (elem[2],
elem[3]))
self.gdatafile.close()
def plot(self):
self._writefiles()
gplot = subprocess.Popen(self.gnuplot + " " +
self.gprog[1],\
shell=True, stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
print "Plotting data (%s)..." % self.outfname
so, se = gplot.communicate()
if se:
print "Gnuplot problem output:", se
os.remove(self.gprog[1])
os.remove(self.gdata[1])
if __name__ == "__main__":
try:
fname = sys.argv[1]
except IndexError:
print "Specify filename with data as first argument."
sys.exit(1)
except Exception, e:
print "Error:", e
sys.exit(2)
values = MovingAvg(fname)
values.extrfromfile()
values.calc_sma(smalen = 10)
(reslist, smalist, maxval) = values.return_results()
gp = GnuplotWrapper(smalist, maxval)
gp.plot()
Example data:
08-11-19 12:41 0.11 0.45 0.40 2/158 27091
08-11-19 12:42 0.08 0.43 0.39 9/171 27113
08-11-19 12:43 0.57 0.53 0.43 4/163 12350
08-11-19 12:44 0.21 0.43 0.40 8/176 12422
08-11-19 12:45 0.08 0.36 0.37 6/166 12507
08-11-19 12:46 0.66 0.51 0.43 4/163 30147
08-11-19 12:47 0.32 0.43 0.40 4/163 30195
08-11-19 12:48 0.12 0.35 0.37 9/174 30263
08-11-19 12:49 0.47 0.43 0.40 6/162 15513
08-11-19 12:50 0.17 0.35 0.37 2/159 15562
08-11-19 12:51 0.11 0.30 0.35 6/162 15613
08-11-19 12:52 0.68 0.47 0.41 5/161 779
08-11-19 12:53 0.25 0.38 0.38 4/161 859
08-11-19 12:54 0.16 0.33 0.36 9/174 927
08-11-19 12:55 0.62 0.44 0.39 5/166 18633
08-11-19 12:56 0.36 0.42 0.39 6/162 18672
08-11-19 12:57 0.23 0.37 0.37 5/163 18757
08-11-19 12:58 0.56 0.46 0.40 4/161 4026
08-11-19 12:59 0.26 0.39 0.37 4/220 4134
08-11-19 13:00 0.16 0.33 0.35 7/169 4232
08-11-19 13:01 0.56 0.44 0.39 1/162 21882
08-11-19 13:02 0.22 0.37 0.36 4/161 21921
08-11-19 13:03 0.14 0.32 0.35 5/163 21972
08-11-19 13:04 0.78 0.53 0.42 5/173 7246
08-11-19 13:05 0.34 0.45 0.40 5/163 7299
08-11-19 13:06 0.20 0.39 0.38 5/167 7369
08-11-19 13:07 0.54 0.48 0.41 5/165 24993
08-11-19 13:08 0.25 0.40 0.38 8/174 25071
08-11-19 13:09 0.18 0.36 0.36 6/168 25148
08-11-19 13:10 0.54 0.45 0.39 5/172 10397
08-11-19 13:11 0.27 0.39 0.37 4/167 10440
08-11-19 13:12 0.22 0.36 0.36 5/168 10517
08-11-19 13:13 1.05 0.67 0.47 4/169 28210
08-11-19 13:14 0.46 0.56 0.44 6/172 28266
08-11-19 13:15 0.17 0.45 0.41 6/172 28346
08-11-19 13:16 0.63 0.55 0.45 5/171 13597
08-11-19 13:17 0.42 0.53 0.45 4/172 13679
08-11-19 13:18 0.20 0.44 0.42 5/172 13735
08-11-19 13:19 0.59 0.54 0.45 4/170 31370
08-11-19 13:20 0.21 0.44 0.42 5/172 31431
08-11-19 13:21 0.08 0.36 0.40 6/172 31511
08-11-19 13:22 0.64 0.51 0.45 4/172 16756
08-11-19 13:23 0.23 0.42 0.42 4/172 16804
08-11-19 13:24 0.13 0.35 0.40 9/185 16881
08-11-19 13:25 0.64 0.48 0.44 5/174 2074
08-11-19 13:26 0.26 0.40 0.41 4/172 2120
08-11-19 13:27 0.14 0.34 0.39 5/174 2176
08-11-19 13:28 0.64 0.47 0.43 6/174 19887
08-11-19 13:29 0.23 0.38 0.40 4/172 19972
08-11-19 13:30 0.12 0.32 0.38 4/182 20042
08-11-19 13:31 0.80 0.51 0.44 6/174 5302
08-11-19 13:32 0.29 0.42 0.41 6/173 5360
08-11-19 13:33 0.11 0.34 0.38 5/174 5446
08-11-19 13:34 0.46 0.42 0.40 4/172 23086
08-11-19 13:35 0.22 0.36 0.38 5/172 23137
08-11-19 13:36 0.28 0.38 0.39 7/175 23188
08-11-19 13:37 0.62 0.49 0.42 4/172 8464
08-11-19 13:38 0.28 0.41 0.40 4/172 8512
08-11-19 13:39 0.10 0.33 0.37 6/175 8571
08-11-19 13:40 0.51 0.42 0.39 6/173 26216
08-11-19 13:41 0.18 0.34 0.37 4/172 26296
08-11-19 13:42 0.07 0.28 0.34 6/173 26345
08-11-19 13:43 0.49 0.38 0.37 5/172 11590
08-11-19 13:44 0.25 0.33 0.35 6/173 11639
08-11-19 13:45 0.09 0.26 0.33 8/179 11747
08-11-19 13:46 0.58 0.40 0.37 4/175 29375
08-11-19 13:47 0.27 0.34 0.35 4/175 29423
08-11-19 13:48 0.10 0.28 0.33 6/176 29472
08-11-19 13:49 0.55 0.40 0.37 4/174 14752
08-11-19 13:50 0.20 0.33 0.34 4/170 14803
08-11-19 13:51 0.15 0.28 0.32 6/171 14852
08-11-19 13:52 0.52 0.39 0.36 5/172 32488
08-11-19 13:53 0.23 0.33 0.34 4/170 32566
08-11-19 13:54 0.08 0.27 0.32 6/173 32624
08-11-19 13:55 0.50 0.38 0.35 4/172 17880
08-11-19 13:56 0.24 0.32 0.33 6/174 17930
08-11-19 13:57 0.09 0.26 0.31 9/184 18044
08-11-19 13:58 0.51 0.37 0.35 4/167 3383
08-11-19 13:59 0.18 0.30 0.32 4/167 3449
08-11-19 14:00 0.10 0.26 0.30 6/171 3509
08-11-19 14:01 0.50 0.36 0.34 7/168 21261
08-11-19 14:02 0.25 0.32 0.32 7/176 21318
08-11-19 14:03 0.09 0.26 0.30 6/169 21359
08-11-19 14:04 0.59 0.39 0.34 5/170 6613
08-11-19 14:05 0.21 0.31 0.32 5/251 6774
08-11-19 14:06 0.08 0.25 0.29 5/170 6832
08-11-19 14:07 0.59 0.39 0.34 4/168 24469
08-11-19 14:08 0.28 0.34 0.32 6/179 24530
08-11-19 14:09 0.10 0.27 0.30 6/172 24600
08-11-19 14:10 0.55 0.40 0.34 4/175 9857
08-11-19 14:11 0.20 0.32 0.31 4/172 9898
08-11-19 14:12 0.07 0.26 0.29 8/183 9967
08-11-19 14:13 0.51 0.37 0.33 4/169 27631
08-11-19 14:14 0.19 0.30 0.30 4/175 27705
08-11-19 14:15 0.18 0.27 0.29 6/171 27763
08-11-19 14:16 0.65 0.41 0.34 5/171 13019
08-11-19 14:17 0.24 0.34 0.31 7/176 13109
08-11-19 14:18 0.09 0.27 0.29 5/171 13155
08-11-19 14:19 0.46 0.37 0.32 4/169 30800
08-11-19 14:20 0.21 0.31 0.30 5/171 30852
08-11-19 14:21 0.12 0.27 0.28 6/171 30937
08-11-19 14:22 0.52 0.38 0.32 4/167 16174
08-11-19 14:23 0.25 0.32 0.30 4/167 16225
08-11-19 14:24 0.09 0.26 0.28 9/180 16293
08-11-19 14:25 0.47 0.36 0.31 4/175 1505
08-11-19 14:26 0.20 0.30 0.29 4/167 1550
08-11-19 14:27 0.07 0.25 0.27 5/172 1607
08-11-19 14:28 0.73 0.42 0.33 6/169 19351
08-11-19 14:29 0.87 0.55 0.37 4/143 19654
08-11-19 14:30 0.64 0.58 0.40 4/175 19833
08-11-19 14:31 0.91 0.68 0.45 4/166 5087
08-11-19 14:32 0.33 0.56 0.42 6/167 5148
08-11-19 14:33 0.12 0.45 0.39 5/169 5231
08-11-19 14:34 0.56 0.54 0.42 5/247 22939
08-11-19 14:35 0.20 0.44 0.39 7/166 23002
08-11-19 14:36 0.07 0.36 0.36 6/167 23081
08-11-19 14:37 0.49 0.45 0.39 5/169 8374
08-11-19 14:38 0.29 0.40 0.38 6/168 8468
08-11-19 14:39 0.15 0.34 0.35 5/167 8503
08-11-19 14:40 1.07 0.58 0.43 5/177 26164
08-11-19 14:41 0.39 0.47 0.40 4/165 26233
08-11-19 14:42 0.26 0.41 0.39 6/166 26292
08-11-19 14:43 0.68 0.53 0.43 4/163 11539
08-11-19 14:44 0.25 0.43 0.40 6/165 11643
08-11-19 14:45 0.09 0.35 0.37 6/166 11727
08-11-19 14:46 0.55 0.46 0.40 6/167 29357
08-11-19 14:47 0.20 0.37 0.37 4/164 29410
08-11-19 14:48 0.07 0.30 0.35 6/169 29477
08-11-19 14:49 0.70 0.48 0.41 4/174 14803
08-11-19 14:50 0.26 0.39 0.38 4/173 14853
08-11-19 14:51 0.09 0.31 0.35 6/172 14902
08-11-19 14:52 0.52 0.40 0.38 5/173 32535
08-11-19 14:53 0.19 0.33 0.35 4/171 32644
08-11-19 14:54 0.20 0.30 0.34 4/179 32760
08-11-19 14:55 0.74 0.46 0.39 4/168 18204
08-11-19 14:56 0.33 0.39 0.37 5/167 18264
08-11-19 14:57 0.12 0.32 0.34 6/169 18357
08-11-19 14:58 0.50 0.42 0.37 8/174 3640
08-11-19 14:59 0.18 0.34 0.35 4/169 3715
08-11-19 15:00 0.06 0.27 0.33 7/173 3789
08-11-19 15:01 0.56 0.40 0.36 6/175 21475
08-11-19 15:02 0.24 0.34 0.35 7/179 21521
08-11-19 15:03 0.55 0.39 0.36 5/168 22067
08-11-19 15:04 0.76 0.49 0.40 6/171 7328
08-11-19 15:05 0.58 0.46 0.39 4/165 7493
08-11-19 15:06 0.21 0.38 0.36 5/167 7634
08-11-19 15:07 0.60 0.48 0.40 4/165 25276
08-11-19 15:08 0.22 0.39 0.37 9/177 25377
08-11-19 15:09 0.18 0.35 0.35 6/171 25491
08-11-19 15:10 0.58 0.46 0.39 4/173 10780
08-11-19 15:11 0.21 0.37 0.36 4/169 10825
08-11-19 15:12 0.11 0.32 0.34 6/170 10875
08-11-19 15:13 0.43 0.39 0.36 4/168 28534
08-11-19 15:14 0.15 0.31 0.34 4/173 28589
08-11-19 15:15 0.06 0.25 0.32 6/170 28642
08-11-19 15:16 0.52 0.38 0.36 5/169 13930
08-11-19 15:17 0.26 0.32 0.34 5/173 14048
08-11-19 15:18 0.09 0.26 0.31 5/170 14095
08-11-19 15:19 0.55 0.39 0.35 4/167 31724
08-11-19 15:20 0.34 0.35 0.34 5/179 31814
08-11-19 15:21 0.16 0.29 0.32 6/169 31911
08-11-19 15:22 0.64 0.43 0.37 6/167 17277
08-11-19 15:23 0.29 0.36 0.35 4/163 17388
08-11-19 15:24 0.16 0.31 0.33 6/166 17460
08-11-19 15:25 0.48 0.40 0.36 3/188 2923
08-11-19 15:26 0.62 0.47 0.38 4/195 3159
08-11-19 15:27 0.81 0.55 0.41 6/207 3261
08-11-19 15:28 0.77 0.61 0.44 5/208 20910
08-11-19 15:29 0.32 0.51 0.42 4/206 20992
08-11-19 15:30 0.11 0.41 0.39 6/208 21046
08-11-19 15:31 0.53 0.50 0.42 7/204 6333
08-11-19 15:32 0.19 0.40 0.39 5/205 6389
08-11-19 15:33 0.12 0.34 0.37 3/206 6500
08-11-19 15:34 0.43 0.41 0.39 6/198 24105
08-11-19 15:35 0.20 0.35 0.36 5/204 24164
08-11-19 15:36 0.07 0.28 0.34 6/201 24210
08-11-19 15:37 0.49 0.39 0.37 5/206 9525
08-11-19 15:38 0.22 0.33 0.35 4/197 9564
08-11-19 15:39 0.14 0.28 0.33 6/198 9613
08-11-19 15:40 0.48 0.38 0.36 6/199 27246
08-11-19 15:41 0.17 0.31 0.34 4/199 27345
08-11-19 15:42 0.06 0.25 0.31 4/235 27433
08-11-19 15:43 0.50 0.36 0.35 5/199 12705
08-11-19 15:44 0.18 0.29 0.33 12/211 12765
08-11-19 15:45 0.06 0.24 0.30 6/202 12839
08-11-19 15:46 0.54 0.37 0.34 6/199 30464
08-11-19 15:47 0.20 0.30 0.32 4/199 30512
08-11-19 15:48 0.11 0.25 0.30 10/209 30587
08-11-19 15:49 0.47 0.35 0.33 6/198 15867
08-11-19 15:50 0.17 0.29 0.31 4/198 15918
08-11-19 15:51 0.13 0.25 0.29 6/199 15967
08-11-19 15:52 0.52 0.36 0.33 5/200 1142
08-11-19 15:53 0.19 0.29 0.30 4/198 1220
08-11-19 15:54 0.10 0.25 0.28 8/206 1277
08-11-19 15:55 0.51 0.36 0.32 5/203 19023
08-11-19 15:56 0.24 0.31 0.30 6/198 19056
08-11-19 15:57 0.16 0.27 0.28 5/202 19155
08-11-19 15:58 0.62 0.41 0.33 7/201 4452
08-11-19 15:59 0.23 0.33 0.31 6/210 4509
08-11-19 16:00 0.14 0.28 0.29 6/202 4551
08-11-19 16:01 0.53 0.39 0.32 4/200 22211
08-11-19 16:02 0.29 0.35 0.31 5/209 22271
08-11-19 16:03 0.11 0.28 0.29 5/200 22325
08-11-19 16:04 0.38 0.34 0.31 6/199 7612
08-11-19 16:05 0.24 0.31 0.30 5/199 7698
08-11-19 16:06 0.15 0.27 0.28 5/200 7754
08-11-19 16:07 0.57 0.39 0.32 5/208 25407
08-11-19 16:08 0.21 0.32 0.29 5/199 25449
08-11-19 16:09 0.07 0.26 0.27 6/199 25530
08-11-19 16:10 0.54 0.38 0.31 5/199 10809
08-11-19 16:11 0.25 0.33 0.30 4/198 10857
08-11-19 16:12 0.09 0.26 0.27 6/199 10906
08-11-19 16:13 0.42 0.35 0.30 4/198 28564
08-11-19 16:14 0.15 0.28 0.28 4/198 28614
08-11-19 16:15 0.09 0.24 0.26 5/199 28666
08-11-19 16:16 0.50 0.36 0.30 8/198 13948
08-11-19 16:17 0.23 0.30 0.28 4/203 14036
08-11-19 16:18 0.16 0.26 0.27 6/199 14080
08-11-19 16:19 0.48 0.36 0.30 7/198 31710
08-11-19 16:20 0.18 0.29 0.27 6/202 31784
08-11-19 16:21 0.06 0.23 0.26 6/199 31846
08-11-19 16:22 0.56 0.37 0.30 4/198 17118
08-11-19 16:23 0.20 0.30 0.28 4/198 17166
08-11-19 16:24 0.11 0.25 0.26 7/200 17221
08-11-19 16:25 0.61 0.40 0.31 6/199 2475
08-11-19 16:26 0.30 0.34 0.29 4/195 2629
08-11-19 16:27 0.11 0.27 0.27 6/196 2716
08-11-19 16:28 0.73 0.44 0.33 7/197 20385
08-11-19 16:29 0.27 0.36 0.30 4/198 20467
08-11-19 16:30 0.16 0.31 0.29 5/199 20520
08-11-19 16:31 0.53 0.41 0.32 6/197 5810
08-11-19 16:32 0.19 0.33 0.30 5/198 5870
08-11-19 16:33 0.11 0.28 0.28 6/206 5972
08-11-19 16:34 0.47 0.38 0.31 4/196 23579
08-11-19 16:35 0.17 0.31 0.29 5/198 23631
08-11-19 16:36 0.12 0.26 0.27 7/199 23681
08-11-19 16:37 1.03 0.52 0.36 5/201 9044
08-11-19 16:38 0.51 0.47 0.35 4/203 9118
08-11-19 16:39 0.23 0.40 0.33 6/206 9204
08-11-19 16:40 0.60 0.50 0.37 11/211 26871
08-11-19 16:41 0.22 0.40 0.35 4/203 26943
08-11-19 16:42 0.08 0.33 0.32 6/209 27037
08-11-19 16:43 0.70 0.47 0.37 5/203 12342
08-11-19 16:44 0.62 0.47 0.37 5/207 12424
08-11-19 16:45 0.26 0.39 0.35 6/206 12510
08-11-19 16:46 0.66 0.51 0.39 4/203 30135
08-11-19 16:47 0.31 0.43 0.37 5/206 30196
08-11-19 16:48 0.17 0.36 0.35 6/205 30243
08-11-19 16:49 0.62 0.49 0.39 5/206 15559
08-11-19 16:50 0.34 0.43 0.37 7/206 15604
08-11-19 16:51 0.12 0.35 0.35 7/209 15655
08-11-19 16:52 0.52 0.44 0.38 5/202 818
08-11-19 16:53 0.22 0.37 0.36 4/206 912
08-11-19 16:54 0.08 0.30 0.33 5/201 955
Regards,
mk
Jean-Paul Calderone
>Hello everyone,
>
>I wrote the following program mainly for educational purpose of improving my
>Python programming skills -- would Pythonistas here please look at it and
>point out areas that could use improvement?
>
>This thing analyzes the stored load average record file, calculates simple
>moving average and produces Gnuplot program to plot the thing.
>
>Constructive criticism is welcome.
>
>#!/usr/bin/python
>
>import re
>import sys
>import tempfile
>import subprocess
>import os
>
>class MovingAvg(object):
You should have a docstring here. It should describe the overall purpose
of the MovingAvg class and it should probably document the meaning of each
attribute instances of the class have.
> def __init__(self, fname):
> try:
> self.fo = open(fname)
> except IOError, e:
> print "Problem with opening file:", e
> sys.exit(4)
This isn't very good. Your MovingAvg class is otherwise relatively general-
purpose and re-usable, but this makes it unusable instead. You shouldn't
use print to report errors in general code and you shouldn't turn specific
exceptions into SystemExit. This kind of whole-program error handling
belongs in a different layer if your code. Your moving average code should
just focus on computing a moving average.
Also, filenames are rather clunky. Another improvement here would be to
pass in a file object which the calling code has already opened. Or, better
still, just pass in an iterator of data. Do the file handling and the
parsing in a different layer. Then you'll be able to re-use your moving
average class with data files in a different format - all you'll have to do
is write another parser for the new format.
> self.reslist = []
> self.smalist = []
> self.maxval = 0
>
> def extrfromfile(self):
There should be a docstring here describing what this method does.
> vre = re.compile("(\d+-\d+-\d+) (\d\d:\d\d) (\d+\.\d+)")
> for line in self.fo:
> res = vre.search(line)
> if res:
> self.reslist.append({'day':res.group(1),\
> 'time':res.group(2),\
> 'val':float(res.group(3))})
The trailing backslashes on the previous lines are superfluous. The code
is just as valid without them.
> def calc_sma(self, smalen=4):
Another missing docstring. Make sure you also document the meaning of the
parameter the method accepts.
> if smalen == 0:
> raise AssertionError, "Moving Average sample length
>cannot be 0"
> if not isinstance(smalen, int):
> raise AssertionError, "Moving Average sample length
>has to be integer"
The conventional way to write the previous lines would be:
assert smallen != 0, "Moving Average sample length cannot be 0"
assert isinstance(smallen, int), "Moving Average Sample length has to be integer"
However, I would just leave them out. If you document the meaning of the
smalen parameter, then callers will know it can't be 0 and must be an int.
> total = 0
Superfluous line above.
> total = sum( [ x['val'] for x in self.reslist[0:smalen] ] )
> sma = total / smalen
> smaidx = int(smalen/2)
> self.smalist.append((self.reslist[0]['day'],\
> self.reslist[0]['time'],\
> self.reslist[0]['val'],\
> self.reslist[0]['val']))
Superfluous backslashes again.
> for i in range(smalen, len(self.reslist)):
> curval = self.reslist[i]['val']
> self.maxval = max(self.maxval, curval)
> total += curval
> total -= self.reslist[i - smalen]['val']
> sma = total / smalen
> smaidx += 1
> self.smalist.append((self.reslist[smaidx]['day'],\
> self.reslist[smaidx]['time'],\
> self.reslist[smaidx]['val'],\
> sma))
And again.
>
> def return_results(self):
Missing docstring.
> return (self.reslist, self.smalist, self.maxval)
Generally, I wonder whether MovingAvg should just be a function rather
than a class. The only place it maintains state it does so confusingly.
I would just make the input data a parameter to calc_sma and have it return
the results it computes. Drop the rest of the class and make calc_sma a free
function.
>class GnuplotWrapper(object):
Docstring.
> def __init__(self, smalist, maxval, outfname = "calc.png",
>graphlen=640, graphheight=480, gnuplot = '/usr/bin/gnuplot'):
Again, the comment about filenames. You could just make outfname a file-like
object and skip the naming.
> self.outfname = outfname
> self.smalist = smalist
> self.gnuplot = gnuplot
> self.gprog = None
> self.gdata = None
> self.graphlen = graphlen
> self.graphheight = graphheight
>
> def _writefiles(self):
Docstring.
> self.gprog = tempfile.mkstemp()
> self.gdata = tempfile.mkstemp()
> self.gprogfile = open(self.gprog[1], 'wb')
> self.gdatafile = open(self.gdata[1], 'wb')
> labelnum = int(self.graphlen / 110)
> labelstep = int(len(self.smalist) / labelnum)
> labels = []
> for i in range(0, len(self.smalist), labelstep):
> labels.append("\"%s %s\" %d" % (self.smalist[i][0],
>self.smalist[i][1], i))
> labelstr = ", ".join(labels)
>
> self.gprogfile.write("""set terminal png size %d, %d
>set style line 1 lt 1 lw 2
>set style line 2 lt 2 lw 2
>set output "%s"
>set xtics(%s)
>set yrange [0:%f]
>set y2range [0:%f]
>plot "%s" using 1 with lines ls 1 title "orig" axes x1y1, "%s" using 2 with
>lines ls 2 title "Moving Average" axes x1y2
>""" % (self.graphlen, self.graphheight, self.outfname, labelstr,
>float(maxval), float(maxval),\
Superfluous backslash.
> self.gdata[1], self.gdata[1]) )
> self.gprogfile.close()
> for elem in self.smalist:
> self.gdatafile.write("%f, %f\n" % (elem[2],
>elem[3]))
> self.gdatafile.close()
>
> def plot(self):
Docstring.
> self._writefiles()
> gplot = subprocess.Popen(self.gnuplot + " " +
>self.gprog[1],\
> shell=True, stdout=subprocess.PIPE,
>stderr=subprocess.PIPE)
You should *avoid* using shell=True with subprocess.Popen. There's no
reason to use it here, as far as I can tell.
> print "Plotting data (%s)..." % self.outfname
> so, se = gplot.communicate()
> if se:
> print "Gnuplot problem output:", se
> os.remove(self.gprog[1])
> os.remove(self.gdata[1])
Reporting the gnuplot failure with print is not the best thing. Reporting
errors to users should be done at a separate layer. This is your gnuplot
wrapper - its job is to wrap gnuplot, not to talk to the user. If there's
a problem, make it available via some documented API (for example, the plot
method might raise an exception if there is a gnuplot problem). Handle the
exception at a higher level in your application where you know it's correct
to print things for the user to read.
>
>
>if __name__ == "__main__":
> try:
> fname = sys.argv[1]
> except IndexError:
> print "Specify filename with data as first argument."
> sys.exit(1)
> except Exception, e:
> print "Error:", e
> sys.exit(2)
What is the second exception handler doing? There is no expected other
exception from the code being protected, so you should just get rid of
this handler. If, through some crazy accident, another exception gets
raised, Python will take care of reporting it to the user, and it will
do so with far more information - information that will make your job of
debugging the problem vastly easier.
> values = MovingAvg(fname)
> values.extrfromfile()
> values.calc_sma(smalen = 10)
> (reslist, smalist, maxval) = values.return_results()
> gp = GnuplotWrapper(smalist, maxval)
> gp.plot()
>
The most significant thing missing from this code is unit tests. Developing
automated tests for your code will help you learn a lot.
Jean-Paul
mk
<snip>
> The most significant thing missing from this code is unit tests.
> Developing automated tests for your code will help you learn a lot.
Thanks for all the remarks, I'll restructure my code. Probably the
biggest mistake was not keeping "moving average" class and others
focused on its respective jobs only.
However, unit tests are a tougher cookie, do you have any resource
that's truly worth recommending for learning unit tests? The only
resource I found that was talking at some length about it was "Dive into
Python" and I was wondering if there's anything better out there...
Regards,
mk
Jean-Paul Calderone
I haven't read it, but a skilled colleague of mine recommends
<http://www.amazon.com/xUnit-Test-Patterns-Refactoring-Addison-Wesley/dp/0131495054/>.
Jean-Paul
Scott David Daniels
> Jean-Paul Calderone wrote:
> <snip>
>> The most significant thing missing from this code is unit tests.
>> Developing automated tests for your code will help you learn a lot.
>
> that's truly worth recommending for learning unit tests? The only
> resource I found that was talking at some length about it was "Dive into
> Python" and I was wondering if there's anything better out there...
Do the "Dive In" thing. You'll quickly develop instincts.
Start with the simplest possible test of your function,
run it, and fix your code until it passes. Add another test,
and loop. The simplest possible test will unfortunately need
a bit of bolerplate, but adding tests is easy.
to start:
import unittest
import moving_average as ma
class MovingAverageTest(unittest.TestCase):
def test_simplest(self):
'Check the mechanics of getting a simple result'
self.assertEqual(ma.moving_average(3, []), [])
if __name__ == '__main__':
unittest.main()
Once that works, add another test:
def test_simple(self):
self.assertAlmostEqual(ma.moving_average(3, [1, 2, 3]), [2])
Once that works, check something a bit easier to get wrong:
def test_floating(self):
'Check we have reasonable results'
self.assertAlmostEqual(ma.moving_average(3, [1, 1, 3])[0],
1.6666667)
and, as they so often say, "lather, rinse, repeat."
--Scott David Daniels
Scott....@Acm.Org
Terry Reedy
> Jean-Paul Calderone wrote:
> <snip>
>> The most significant thing missing from this code is unit tests.
>> Developing automated tests for your code will help you learn a lot.
>
> biggest mistake was not keeping "moving average" class and others
> focused on its respective jobs only.
I would consider writing the moving-average 'function' as a generator
function. First version takes a sequence and window size as inputs.
Second, slightly more difficult, takes an iterator and window size as
inputs. An exponentially-weighted average generator would, of course,
take an iterator and weight as input.
Aahz
mk <mrk...@gmail.com> wrote:
>
>However, unit tests are a tougher cookie, do you have any resource
>that's truly worth recommending for learning unit tests?
Start with the doctest module.
--
Aahz (aa...@pythoncraft.com) <*> http://www.pythoncraft.com/
Weinberg's Second Law: If builders built buildings the way programmers wrote
programs, then the first woodpecker that came along would destroy civilization.