Re: Code critique please
kai.pet...@gmail.com wrote:
> I just wrote this bit (coming from Pascal)
> if (((xdim / 8) * ydim) + header) <> filesize:
Yeah, either Pascal or Barry Warsaw is your uncle ;)
https://www.python.org/dev/peps/pep-0401/
> and am wondering how seasoned
> Python programmers would have done the same? Anything terribly non-python?
> def RenderByte(draw, byte, x, y):
Please read PEP 8 for naming conventions etc.
The function at the core of your code
> def RenderByte(draw, byte, x, y):
>
> blist = list(bin(byte).lstrip('0b')) # turn byte into list with 8
> elements,
> c = 0# each representing one bit
> for bit in blist:
> if bit:
> draw.point((x + c, y), fcolor)
>
> c += 1
> return
can be fixed/micro-optimised, and if I were to do that I'd precreate a
lookup table that maps every byte (i. e. value in the range 0...255) to a
sequence of offsets
lookup = [
(), # for b"\x00" the inner loop is empty
(0),
(1),
(0, 1),
...
(0, 1, 2, 3, 4, 5, 6, 7), # for b"\xFF" the inner loop comprises
# all 8 bits
]
or to a 8x1 image, but my idea of a "seasoned Python programmer" will try to
keep the big picture in mind -- and that is that handling individual
bits/pixels in Python is inefficient. Therefore many libraries tend to offer
an alternative that is both easier to use and faster.
In this case that's
img = Image.frombytes("1", (width, height), data)
and with the extra comfort of reading the size from the EPD file
from PIL import Image
import struct
EPDFILE = "tmp.epd"
PNGFILE = "tmp.png"
class EPDError(Exception):
pass
with open(EPDFILE, "rb") as f:
header = f.read(16)
width, height, colordepth = struct.unpack("http://www.pervasivedisplays.com/_literature_112691/Developer_Guide_for_Demo_Kit
PS: I thought that Image.frombytes() was already pointed out to you, but I
failed to find it in the archives. So there.
--
https://mail.python.org/mailman/listinfo/python-list
Re: Code critique please
- Original Message -
> From: "kai peters"
> To: python-list@python.org
> Sent: Wednesday, 8 April, 2015 12:43:23 AM
> Subject: Code critique please
>
> I just wrote this bit (coming from Pascal) and am wondering how
> seasoned Python programmers would have done the same? Anything
> terribly non-python?
>
> As always, thanks for all input.
>
> K
>
>
>
> """
> Creates a PNG image from EPD file
> """
>
> import os, sys
> from PIL import Image, ImageFont, ImageDraw
>
> #
> -
> def RenderByte(draw, byte, x, y):
>
> blist = list(bin(byte).lstrip('0b')) # turn byte into list with 8
> elements,
> c = 0# each representing one bit
> for bit in blist:
> if bit:
> draw.point((x + c, y), fcolor)
>
> c += 1
> return
>
> #
Apart from what has been already said, you could rewrite your function
RenderByte this way (untested):
def render_byte(draw, byte, x, y):
for point in [(x+c, y) for (c, bit) in enumerate(bin(byte)[2:]) if int(bit)]:
draw.point(point, fcolor)
it involves important notions in the python language:
- list comprehension, this is the [...] part, where it combines filtering a
list and applying a function to the values
- slicing, bin(byte)[2:] returning the sequence stripped from its 2 first
elements
Additional remarks :
- is fcolor defined ?
- your test "if bit:" was probably wrong as it was testing either "0" or "1"
which are both evaluated to True. In other words, bool("0") == True, bool(0) ==
False
Cheers,
JM
-- IMPORTANT NOTICE:
The contents of this email and any attachments are confidential and may also be
privileged. If you are not the intended recipient, please notify the sender
immediately and do not disclose the contents to any other person, use it for
any purpose, or store or copy the information in any medium. Thank you.
--
https://mail.python.org/mailman/listinfo/python-list
Re: Code critique please
On 2015-04-08 01:54, Mark Lawrence wrote: On 07/04/2015 23:43, kai.pet...@gmail.com wrote: I just wrote this bit (coming from Pascal) and am wondering how seasoned Python programmers would have done the same? Anything terribly non-python? As always, thanks for all input. import os, sys from PIL import Image, ImageFont, ImageDraw As you've had plenty of answers I'll just say that PIL is pretty old now. Are you aware of the fork called Pillow? https://pillow.readthedocs.org/ Pillow uses the name "PIL" for its package name too, in the interest of being drop-in compatible. https://pillow.readthedocs.org/porting-pil-to-pillow.html -- Robert Kern "I have come to believe that the whole world is an enigma, a harmless enigma that is made terrible by our own mad attempt to interpret it as though it had an underlying truth." -- Umberto Eco -- https://mail.python.org/mailman/listinfo/python-list
Re: Code critique please
On 07/04/2015 23:43, kai.pet...@gmail.com wrote: I just wrote this bit (coming from Pascal) and am wondering how seasoned Python programmers would have done the same? Anything terribly non-python? As always, thanks for all input. import os, sys from PIL import Image, ImageFont, ImageDraw As you've had plenty of answers I'll just say that PIL is pretty old now. Are you aware of the fork called Pillow? https://pillow.readthedocs.org/ -- My fellow Pythonistas, ask not what our language can do for you, ask what you can do for our language. Mark Lawrence -- https://mail.python.org/mailman/listinfo/python-list
Re: Code critique please
On Tuesday, 7 April 2015 15:43:44 UTC-7, kai.p...@gmail.com wrote:
> I just wrote this bit (coming from Pascal) and am wondering how seasoned
> Python programmers would have done the same? Anything terribly non-python?
>
> As always, thanks for all input.
>
> K
>
>
>
> """
> Creates a PNG image from EPD file
> """
>
> import os, sys
> from PIL import Image, ImageFont, ImageDraw
>
> #
> -
> def RenderByte(draw, byte, x, y):
>
> blist = list(bin(byte).lstrip('0b')) # turn byte into list with 8
> elements,
> c = 0# each representing one bit
> for bit in blist:
> if bit:
> draw.point((x + c, y), fcolor)
>
> c += 1
> return
>
> #
> -
> def EPD_2_Image(edpfilename, imagefilename):
>
> # get out of here if EPD file not present
> if not os.path.isfile(epdfilename):
> print 'file not found: ' + edpfilename
> return
>
> # is this a valid EPD file?
> filesize = os.path.getsize(epdfilename)
> if (((xdim / 8) * ydim) + header) <> filesize:
> print 'incorrect file size: ' + edpfilename
> return
>
> # blow old destination file away
> if os.path.isfile(imagefilename):
> print 'deleting old dest. file: ' + imagefilename
> os.remove(imagefilename)
>
> print 'processing...'
>
> # set up PIL objects
> img = Image.new('1', (xdim, ydim), bcolor) # '1' = Bi-tonal image
> draw = ImageDraw.Draw(img)
>
> # read entire EPD file into byte array (without the header)
> content = bytearray(open(epdfilename, 'rb').read())[16:]
>
> # image coord origin at top/left according to PIL documentation
> pos = 0
> for y in range(ydim):
> x = 0
> for byte in range(xdim / 8): # 8 pixels 'stuffed' into one byte
> RenderByte(draw, content[pos], x, y)
> pos += 1
> x += 8
>
> img.save(imagefilename) # format is inferred from given extension
> print 'done.'
>
> return
> #
> -
>
> xdim = 1024
> ydim = 1280
> header = 16
> black = 0
> white = 1
> bcolor = black
> fcolor = white
>
> epdfilename = 'c:\\temp\\drawtest2.epd'
> imagefilename = 'c:\\temp\\drawtest2.png'
>
> EPD_2_Image(epdfilename, imagefilename)
Thanks for taking the time to give such detailed suggestions - very much
appreciated!
K
--
https://mail.python.org/mailman/listinfo/python-list
Re: Code critique please
On 07Apr2015 15:43, kai.pet...@gmail.com wrote:
I just wrote this bit (coming from Pascal) and am wondering how seasoned Python
programmers would have done the same? Anything terribly non-python?
As always, thanks for all input.
K
"""
Creates a PNG image from EPD file
"""
import os, sys
from PIL import Image, ImageFont, ImageDraw
# -
def RenderByte(draw, byte, x, y):
blist = list(bin(byte).lstrip('0b')) # turn byte into list with 8 elements,
Remark: .lstrip does not do what you think.
You want:
blist = list(bin(byte)[2:]) # turn byte into list with 8 elements,
to skip over the leading "0b". Otherwise, with lstrip, consider what would
happen with a low value byte, with multiple leading zeroes. (Actually, on
reflection, you might get away with it - but probably not, and anyway would be
fragile against changing the ordering of the bits.)
I'd be popping off the least or most siginificant bit myself with "&" and "<<"
or ">>". It might be wordier, bit it would be more in keeping with the actual
bitwise operations going on.
c = 0# each representing one bit
for bit in blist:
if bit:
draw.point((x + c, y), fcolor)
c += 1
This might be more Pythonic written:
for c, bit in enumerate(blist):
which will emit (0, b0), (1, b1) and so forth for bit 0, bit 1 etc (where bit 0
is the leftmost from your list, etc). Avoids the "c = 0" and the "c += 1" and
also makes your loop robust against adding control flows later which might skip
the final "c += 1" at the end, even by accident.
return
You generally do not need a trailing return with no value; it is implicit.
def EPD_2_Image(edpfilename, imagefilename):
# get out of here if EPD file not present
if not os.path.isfile(epdfilename):
print 'file not found: ' + edpfilename
return
Traditionally one would raise an exception instead of returning. Eg:
if not os.path.isfile(epdfilename):
raise ValueError("missing file: %r" % (epdfilename,))
and have the caller handle the issue. Similarly for all the other pre-checks
below.
Also, it is normal for error messages to be directed to sys.stderr (this allows
them to be logged independently of the program's normal output; for example the
normal output might be sent to a file, but the error messages would continue to
be displayed on screen). So were you to iussue a print statement (instead of an
exception) you would write:
print >>sys.stderr,
or in Python 3:
print(, file=sys.stderr)
# is this a valid EPD file?
filesize = os.path.getsize(epdfilename)
if (((xdim / 8) * ydim) + header) <> filesize:
print 'incorrect file size: ' + edpfilename
return
# blow old destination file away
if os.path.isfile(imagefilename):
print 'deleting old dest. file: ' + imagefilename
os.remove(imagefilename)
print 'processing...'
# set up PIL objects
img = Image.new('1', (xdim, ydim), bcolor) # '1' = Bi-tonal image
draw = ImageDraw.Draw(img)
# read entire EPD file into byte array (without the header)
content = bytearray(open(epdfilename, 'rb').read())[16:]
# image coord origin at top/left according to PIL documentation
pos = 0
for y in range(ydim):
x = 0
for byte in range(xdim / 8): # 8 pixels 'stuffed' into one byte
RenderByte(draw, content[pos], x, y)
pos += 1
x += 8
img.save(imagefilename) # format is inferred from given extension
print 'done.'
return
Again, this "return" can be implicit.
# -
xdim = 1024
ydim = 1280
header = 16
black = 0
white = 1
bcolor = black
fcolor = white
epdfilename = 'c:\\temp\\drawtest2.epd'
imagefilename = 'c:\\temp\\drawtest2.png'
Normally these values would be at the top of your program, and named in
UPPER_CASE to indicate that they are like "constants" in other languages. So
you might put this:
XDIM = 1024
YDIM = 1280
HEADER_SIZE = 16
BLACK = 0
WHITE = 1
BCOLOR = BLACK
FCOLOR = WHITE
EPD_FILENAME = 'c:\\temp\\drawtest2.epd'
IMAGE_FILENAME = 'c:\\temp\\drawtest2.png'
at the top of the script.
I notice you have a bare [16:] in your "content =" line. Should that not say
"[HEADER_SIZE:]" ?
EPD_2_Image(epdfilename, imagefilename)
I tend to write things like this as thought they could become python modules
for reuse. (Often they do; why write something twice?)
So the base of the script becomes like this:
if __name__ == '__main__':
EPD_2_Image(EPD_FILENAME, IMAGE_FILENAME)
In this way, when you invoke the .py file directly __name__ is "__main__" and
your function gets run. But it you move this all into a module which may be
imported, __name__ will be the module name, an so not invoke the main function.
The importing code can then do so as i
Re: Code critique please
On Tue, Apr 7, 2015 at 3:43 PM, wrote:
> I just wrote this bit (coming from Pascal) and am wondering how seasoned
> Python programmers would have done the same? Anything terribly non-python?
>
> As always, thanks for all input.
>
> K
>
>
>
> """
> Creates a PNG image from EPD file
> """
>
> import os, sys
> from PIL import Image, ImageFont, ImageDraw
>
> #
> -
> def RenderByte(draw, byte, x, y):
>
> blist = list(bin(byte).lstrip('0b')) # turn byte into list with 8
> elements,
A couple of comments on this line: lstrip strips based on characters,
and even without that, the 8 elements comment is not correct. bin()
only returns enough to hold the value without leading zeros, and the
lstrip will remove any leading zeros, even if it did produce them.
Additionally, if byte is outside the [0, 255] range, bin could produce
more than 8 elements. Your loop will work in any case, but it may
cause incorrect behavior, and in any case, the comment is wrong.
> c = 0# each representing one bit
> for bit in blist:
> if bit:
> draw.point((x + c, y), fcolor)
>
> c += 1
Rather than keeping a manual counter, this could be better written
using enumerate:
for c, bit in enumerate(blist):
if bit:
draw.point((x + c, y), fcolor)
> return
This return is unneeded. If a function falls off the end, it
implicitly has a bare "return" at its end. Note that a bare return (or
falling off the end) will cause the function to return None (all
functions have a return value).
>
> #
> -
> def EPD_2_Image(edpfilename, imagefilename):
>
> # get out of here if EPD file not present
> if not os.path.isfile(epdfilename):
> print 'file not found: ' + edpfilename
> return
>
> # is this a valid EPD file?
> filesize = os.path.getsize(epdfilename)
> if (((xdim / 8) * ydim) + header) <> filesize:
> print 'incorrect file size: ' + edpfilename
> return
Both of these would probably be better as exceptions, rather than
prints. When running the function via a batch script, you can always
catch the exception and do something with it, while with a print, the
caller will have a hard time determining whether the function worked
or not. This could be important if you ever try to wrap this function
into a GUI (you may want a dialog rather than a command prompt
textural error, on stdout no less), or call the function with
batching, where you may want to report the failures via other means
(collect them all and include them in an e-mail report, for example).
For example, "raise OSError('file not found' + edpfilename)". Also, in
Python, the perfered dogma is "better to ask forgiveness than
permission" (aka, try the operation and let it fail if invalid),
rather than "look before you leap" (validate correct inputs, and fail
early). Basically, in this case, you could just remove the first check
(the second is probably worthwhile), and let the getsize fail if the
file does not exist.
Raising the exceptions will play better with batching (its easier to
detect errors and report them en-mass after processing). Avoiding LBYL
is generally useful for files in case other processes mess with them
after your check.
>
> # blow old destination file away
> if os.path.isfile(imagefilename):
> print 'deleting old dest. file: ' + imagefilename
> os.remove(imagefilename)
This is probably unneeded, and could cause issues with deleting the
file then failing out for various reasons. I generally prefer to leave
files intact until the last possible second. Thi sis nice if the code
fails for some reason.
>
> print 'processing...'
>
> # set up PIL objects
> img = Image.new('1', (xdim, ydim), bcolor) # '1' = Bi-tonal image
> draw = ImageDraw.Draw(img)
>
> # read entire EPD file into byte array (without the header)
> content = bytearray(open(epdfilename, 'rb').read())[16:]
>
> # image coord origin at top/left according to PIL documentation
> pos = 0
> for y in range(ydim):
> x = 0
> for byte in range(xdim / 8): # 8 pixels 'stuffed' into one byte
> RenderByte(draw, content[pos], x, y)
> pos += 1
> x += 8
>
> img.save(imagefilename) # format is inferred from given extension
> print 'done.'
>
> return
> #
> -
>
> xdim = 1024
> ydim = 1280
> header = 16
> black = 0
> white = 1
> bcolor = black
> fcolor = white
By convention, constants in Python are denoted by using all capital
letters with under scores.
>
> epdfilename = 'c:\\temp\\drawtest2.epd'
> imagefilename = 'c:\\temp\\drawtest2.png'
>
> EPD_2_Image(epdfilename, imagefilename)
> --
> https://mail.python.org/mailman/listinfo/pyt
Re: Code critique please
On Tue, Apr 7, 2015 at 4:43 PM, wrote:
> def RenderByte(draw, byte, x, y):
Python function and method names customarily use the
lowercase_with_underscores style.
> blist = list(bin(byte).lstrip('0b')) # turn byte into list with 8
> elements,
There's no guarantee that the resulting list will have 8 elements
here. The result of bin() will use the minimum number of digits needed
to represent the value, which has nothing to do with the size of a
byte. If you want exactly 8, use str.zfill to add zeroes.
Also, lstrip('0b') will remove *all* occurrences of the characters '0'
and 'b' from the left of the string, not just the exact substring
'0b'. So for example, bin(0).lstrip('0b') would return the empty
string since all the characters in the string are either '0' or 'b'.
That said, all this string manipulation to convert from an int to a
list of bits smells a bit funky to me. I'd probably write a generator
to yield the bits from the number. Something like:
def bits(number, length=None):
if length is None:
length = number.bit_length()
for i in range(length):
yield (number >> i) & 1
Now you can iterate directly over bits(byte, 8), or if you really want
a list then you can call list(bits(byte, 8)).
> c = 0# each representing one bit
> for bit in blist:
> if bit:
> draw.point((x + c, y), fcolor)
>
> c += 1
Instead of incrementing c on each loop, use enumerate.
for c, bit in enumerate(blist):
> return
Not needed if you're not returning a value. It's perfectly okay to
just let execution "fall off" the end of the function.
> # get out of here if EPD file not present
> if not os.path.isfile(epdfilename):
> print 'file not found: ' + edpfilename
Recommend using print's function syntax for forward compatibility with Python 3:
print('file not found: ' + edpfilename)
Also consider doing "from __future__ import print_function" at the top
of the file so that print really is a function, although it doesn't
matter in this case.
> xdim = 1024
> ydim = 1280
> header = 16
> black = 0
> white = 1
> bcolor = black
> fcolor = white
Constants, particularly global ones, are conventionally
UPPERCASE_WITH_UNDERSCORES.
> EPD_2_Image(epdfilename, imagefilename)
If you use the construct
if __name__ == '__main__':
EPD_2_Image(epdfilename, imagefilename)
then the function will automatically be called when the file is
executed as a script, but it will also let you import the file from
other modules and call it on your own terms, should you ever desire to
do that.
--
https://mail.python.org/mailman/listinfo/python-list
Code critique please
I just wrote this bit (coming from Pascal) and am wondering how seasoned Python
programmers would have done the same? Anything terribly non-python?
As always, thanks for all input.
K
"""
Creates a PNG image from EPD file
"""
import os, sys
from PIL import Image, ImageFont, ImageDraw
# -
def RenderByte(draw, byte, x, y):
blist = list(bin(byte).lstrip('0b')) # turn byte into list with 8 elements,
c = 0# each representing one bit
for bit in blist:
if bit:
draw.point((x + c, y), fcolor)
c += 1
return
# -
def EPD_2_Image(edpfilename, imagefilename):
# get out of here if EPD file not present
if not os.path.isfile(epdfilename):
print 'file not found: ' + edpfilename
return
# is this a valid EPD file?
filesize = os.path.getsize(epdfilename)
if (((xdim / 8) * ydim) + header) <> filesize:
print 'incorrect file size: ' + edpfilename
return
# blow old destination file away
if os.path.isfile(imagefilename):
print 'deleting old dest. file: ' + imagefilename
os.remove(imagefilename)
print 'processing...'
# set up PIL objects
img = Image.new('1', (xdim, ydim), bcolor) # '1' = Bi-tonal image
draw = ImageDraw.Draw(img)
# read entire EPD file into byte array (without the header)
content = bytearray(open(epdfilename, 'rb').read())[16:]
# image coord origin at top/left according to PIL documentation
pos = 0
for y in range(ydim):
x = 0
for byte in range(xdim / 8): # 8 pixels 'stuffed' into one byte
RenderByte(draw, content[pos], x, y)
pos += 1
x += 8
img.save(imagefilename) # format is inferred from given extension
print 'done.'
return
# -
xdim = 1024
ydim = 1280
header = 16
black = 0
white = 1
bcolor = black
fcolor = white
epdfilename = 'c:\\temp\\drawtest2.epd'
imagefilename = 'c:\\temp\\drawtest2.png'
EPD_2_Image(epdfilename, imagefilename)
--
https://mail.python.org/mailman/listinfo/python-list
Re: Learning Python: Code critique please
On Sun, 22 Jun 2008 08:44:25 -0500, Saul Spatz wrote: > macoovacany wrote: >> http://macoovacany.wordpress.com/ > When I tried to run it, I got all kinds of syntax errors because of > non-ASCII characters; namely, you have fancy left and right single and > double quotes. That's probably WordPress' doing. -- 09 F9 11 02 9D 74 E3 5B D8 41 56 C5 63 56 88 C0 -- pass it on -- http://mail.python.org/mailman/listinfo/python-list
Re: Learning Python: Code critique please
macoovacany wrote: http://macoovacany.wordpress.com/ When I tried to run it, I got all kinds of syntax errors because of non-ASCII characters; namely, you have fancy left and right single and double quotes. Once I replaced these with the ASCII equivalents, it worked fine. I suggest you use a plain ASCII text editor, like the one that comes with IDLE. HTH, Saul -- http://mail.python.org/mailman/listinfo/python-list
Re: Learning Python: Code critique please
http://macoovacany.wordpress.com/ -- http://mail.python.org/mailman/listinfo/python-list
Learning Python: Code critique please
Hello all, I'm trying to learn various programming languages (I did MATLAB at uni), and have decided to start with Python. The programming exercises are derived from Larry O'brien's http://www.knowing.net/ PermaLink,guid,f3b9ba36-848e-43f8-9caa-232ec216192d.aspx">15 Programming Exercises . First one is up with a couple of comments of my own. Any suggestions, corrections, things that I have done wrong, could do better, etc, please feel free to post a comment. Timbo -- http://mail.python.org/mailman/listinfo/python-list
