@elchico: Das Programm scheint mir fehlerhaft zu arbeiten. Ausgehend von dem Bild welches die folgende PBM-Datei beschreibt…
Code: Alles auswählen
P1
10 10
0 0 0 0 1 0 0 0 0 0
0 0 0 0 1 0 0 0 0 0
0 0 0 0 1 0 0 0 0 0
0 0 0 0 1 0 0 0 0 0
0 0 0 0 1 0 0 0 0 0
0 0 0 0 1 0 0 0 0 0
1 0 0 0 1 0 0 0 0 0
0 1 1 1 0 0 0 0 0 0
0 0 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0 0 0
…kommt diese Ausgabe im ersten Fenster heraus:
Code: Alles auswählen
0000000000
0000000000
0000000000
0000000000
0000000000
0000000000
1000000000
0111000000
0000000000
0000000000
Im zweiten Info-Fenster steht dann '[[[5, 1, 1], [5, 7, 1]]]', was immer das auch bedeuten mag.
Das ist zumindest nicht das was ich erwartet hatte. Noch komischere Ausgaben erhält man bei nicht-quadratischen Bildern. Dagegen sollte man das Programm absichern und die entweder richtig verarbeiten oder explizit mit einer Fehlermeldung abweisen.
Die `line()`-Funktion verändert die übergene Liste, deswegen sieht man in der ersten Ausgabe das 'J' nicht mehr. Das würde ich als Fehler ansehen.
Die GUI ist durch den Hinweistext deutlich breiter als sie sein müsste weil der die erste Spalte des Grid-Layouts unnötig breit macht, und das wo doch die Zelle rechts daneben leer ist. Labels die links von dazugehörigen Eingabemöglichkeiten stehen werden für gewöhnlich auch rechtsbündig gesetzt damit zwischen dem Label und dem Eingabefeld kein langer Leerraum entsteht der die Zuordnung erschwert, insbesondere wenn man mehrere ”Zeilen” hätte.
Es wäre schön wenn das einzige Eingabefeld automatisch den Fokus bekäme und man nicht erst reinklicken müsste. Mit dem Modul `tkFileDialog` gäbe es ausserdem eine bequemere Möglichkeit eine Datei auszuwählen als den Namen komplett per Hand eintippen zu müssen.
Nochmal ein paar Anmerkungen zum Quelltext:
Die Einrückung ist nicht vier Leerzeichen pro Ebene.
Das `time`-Modul wird importiert aber gar nicht verwendet. Bei `Tkinter` wird ein Sternchenimport verwendet der rund 190 Namen in den Modulnamensraum holt von denen dann nur ein paar wenige tatsächlich benutzt werden.
Während es bei `Tkinter` üblich ist das Modul unter dem kurzen Namen `tk` anzusprechen sollte man nicht generell Module unter Kürzeln importieren. Sonst hat man am Ende das Programm voller kryptischer Kurznamen. Das gilt auch für andere Werte als Module. Zum Beispiel `img` sollte eher `image` heissen. Und kryptische Anhängsel sind auch nicht gut, denn selbst als jemand der es selber geschrieben hat, weiss man nach einem Jahr vielleicht nicht mehr was `a` oder `n` in `a_img` oder `n_img` eigentlich bedeuten soll. Namen sollen dem Leser *klar* und *direkt* vermitteln was der Wert dahinter im Kontext des Programms bedeutet. Auch sollte man keine Namen durchnummerieren. Da hat man sich entweder nicht genug Gedanken um einen passenden Namen gemacht, oder man möchte dort eigentlich keine einzelnen Namen haben sondern die Werte in einer Datenstruktur zusammenfassen. Aber bei `explanation1` ist das ”vorsorgliche” Nummerieren einfach nur sinnlos. `insert_path` ist ein schlechter Name für ein `Entry`-Objekt. Der Leser würde hinter dem Namen eher einen Pfad-/Dateinamen erwarten.
``global`` ist Böse™. In diesem Programm kann man das noch leicht vermeiden in dem man `functools.partial()` oder einen `lambda`-Ausdruck verwendet, aber eigentlich sind GUIs ein Grund für objektorientierte Programmierung (OOP), und wenn man OOP verwendet gibt es keine Rechtfertigung für ``global`` mehr.
`img()` ist ein schlechter Name für eine Funktion. Nicht nur das es ein kryptische Abkürzung ist, es ist auch die selbe Abkürzung die Du für ein Bild verwendest. Das gleiche Problem gibt es mit dem Namen der `line()`-Funktion. Wenn man die Funktion `line` nennt wie würde man denn dann einen Wert nennen der eine Zeile repräsentiert? Darum bennennt man Funktionen (und Methoden) nach Tätigkeiten um sie besser von ”passiven” Werten abzugrenzen. Also Beispielsweise `load_image()` um ein Zeile wie diese schreiben zu können: ``image = load_image('test.png')``.
Statt das was eine Funktion macht als Kommentar zu setzen, könnte man es als Docstring schreiben damit es über `help()` verfügbar ist und man von Dokumentationswerkzeugen aus darauf zugreifen kann. Bei `img()` und `line()` stimmen die Angaben im Kommentar zudem nicht. `img()` liefert keine Liste mit 0/1 Werten und auch keine 3D-Liste und `line()` liefert nicht Linienlänge und Richtung als Array sondern eine Liste mit Linien die als Start- und Endpunkt beschrieben sind (und den jeweiligen Pixelwert (0/1) an dem Punkt enthalten).
Zu der `img()`-Funktion hatte ich ja schon mal geschrieben das es sehr ungünstig ist eine von der Natur der Sache her zweidimensionale Datenstruktur in einer eindimensionsionalen Struktur mit regelmässig eingefügten speziellen Markierungswerten zu speichern. Das macht die ganze Weiterverarbeitung unnötig schwer. Zudem sind die Koordinaten in den Elementen auch redundant, denn die lassen sich aus der Position der Elemente berechnen wenn man sie benötigt. Ich würde in der Funktion einfach nur ein zweidimensionales Array mit Wahrheitswerten erstellen. Das wäre ein Einzeiler! Stattdessen müsste ich noch mal vier Zeilen aufwenden um das umständliche Format welches Du verwendest daraus zu machen.
Bei der `line()`-Funktion ist mir auf Anhieb nicht klar was die überhaupt macht, beziehungsweise machen soll.
`data` ist ein nichtssagender Name und irgendwie auch unpassend für ein Flag das nur zwei Werte annehmen kann. Und an der Stelle sollte man dann auch besser `True` und `False` statt 0 und 1 nehmen, denn dann ist sofort klar das der Name sehr wahrscheinlich nie an 42 oder eine andere Zahl gebunden werden wird.
Was man niemals nicht machen sollte sind nackte ``except:`` ohne die konkrete(n) Ausnahme(n) die man dort erwartet. Da wird dann *alles* behandelt, ob das ein `NameError` oder `AttributeError` ist weil man sich irgendwo innerhalb des ``try``-Blocks vertippt hat, oder ein `ValueError` weil irgendwo etwas einen Wert ausserhalb des erlaubten Wertebereichs verwendet hat, oder ein `MemoryError` weil der Arbeitsspeicher aufgebraucht ist, oder ein `KeyboardInterrupt` weil der Benutzer Strg+C in der Konsole gedrückt hat, oder *irgendetwas mit dem man gar nicht gerechnet hat*. Ich vermute jetzt einfach mal das Du in beiden Fällen ausschliesslich auf `IndexError` reagieren wolltest, und alle anderen Ausnahmen gerne *sehen* möchtest weil da dann irgend etwas unvorhergesehenes schief gelaufen ist.
Wenn man die äussere Ausnahmebehandlung anders setzt kann man sich das `data`-Flag sparen. Einfach *um* eine ``while True``-Schleife gesetzt und man braucht in der Funktion einen Namen weniger.
Den Namen `first_element` kann man sich auch sparen, denn auch hier könnte man eine ”Endlosschleife” schreiben die man mit ``break`` abbrechen kann wenn man die erste 1 gefunden hat. Und man müsste den Index nach der Schleife nicht wieder um 1 verringern weil dann wirklich beim Fundindex schon abgebrochen wird.
Das am Anfang jedes Schleifendurchlaufs der äussersten ``while``-Schleife ganz von vorne in den Daten nach der ersten 1 gesucht wird ist ineffizient. Da innerhalb des Algorithmus niemals eine neue 1 gesetzt wird, kann man die Suche an der letzten Fundstelle beginnen, statt jedes mal ganz von vorne zu suchen.
Das ``else`` beim ``while`` macht keinen Sinn weil diese Schleife in keinem Fall durch ein ``break`` verlassen wird. Das was im ``else``-Zweig steht, wird in jedem Fall ausgeführt, also ist der Code weniger komplex wenn man es einfach hinter die ``while``-Schleife schreibt. Dann ist es auch einfacher die empfohlene 80 Zeichen pro Zeile einzuhalten in dem doch recht tief verschachtelten Quelltext.
`list_length` wird in jedem Schleifendurchlauf berechnet obwohl sich der Wert dabei nicht ändert.
Das was Du `it` nennst und mit ``# iterator`` kommentierst ist kein Iterator sondern eine ganze Zahl, und zwar der y-Wert zum Pixel am Index `start_point`. Das hier eine 1 abgezogen werden muss ist der unglücklichen Entscheidung geschuldet den Ursprung des Koordinatensystems nicht bei (0, 0) sondern bei (1, 1) zu setzen. Das Informatiker bei 0 anfangen zu zählen hat einen Grund: Es rechnet sich in den meisten Fällen besser und man muss nicht immer nachdenken ob man einen Wert vor dem Rechnen noch anpassen muss und kann nicht vergessen den nach einer Rechnung wieder in die andere Richtung zu korrigieren.
Wenn man testen möchte ob ein und der selbe Wert einem von zwei anderen Werten entspricht, dann ist ein Test mit ``in`` besser als den zu vergleichenden Wert zweimal zu berechnen und zwei Vergleiche mit ``or`` zu verknüpfen. Insbesondere wenn der zu vergleichende Wert aus einem längeren Ausdruck besteht.
Den `pop()`/`insert()`-Unsinn um einen Wert an einem Index neu zu setzen hatten wir ja schon in einem vorherigen Beitrag. Das hier:
Code: Alles auswählen
x = image[ind][0]
y = image[ind][1]
image.pop(ind)
image.insert(ind, [x, y, 0])
hat für das Programm den gleichen Effekt wie diese Zeile:
Es hat nicht exakt den selben Effekt weil hier keine neue Liste erstellt wird die die alte am Index `ind` ersetzt, aber das ist für dieses Programm egal.
Im ``# line down``-Zweig wird `it` erhöht, dieser Wert wird im folgenden Verlauf aber nirgends verwendet. `out` wird `end_point` zugewiesen, dann aber nur in einer Schleifenbedingung verwendet deren Körper keinen der beiden Werte verändert, man kann sich `out` also sparen und gleich `end_point` verwenden. `it2` wird eine 1 zugewiesen und dann wird der Name nirgends verwendet.
Die innerste Ausnahmebehandlung sieht merkwürdig aus. Der ``try``- und der ``except``-Block enthalten teilweise gleichen Code. Wenn man sie ein wenig anpasst enden beide mit dem exakt gleichen Code was bedeutet das der da nicht zweimal in die beiden Blöcke gehört sondern *einmal* *hinter* das Konstrukt.
Die ``while``-Schleifen zum löschen der Pixel auf der Linie sollten auch eher ``for``-Schleifen sein.
Ich komme dann ungefähr bei so etwas heraus:
Code: Alles auswählen
#!/usr/bin/env python
import Tkinter as tk
import tkMessageBox
import numpy
import PIL.Image
def load_image(filename, threshold=15):
"""Load image into a list of pixel elements.
Each pixel element is a list of the form *[x, y, pixel]*.
The elements are ordered as they appear in the image data from first top
to bottom and left to right with the top left pixel as the first element.
*x* and *y* denote the position of the pixel and *pixel* its color as an
integer: 0 for white and 1 for black.
After the elements for a complete pixel row a special marker element is
present with a coordinate next to the rightmost pixel in that row and a
pixel value of '\\n' (line end character).
The function works with any file which can be read and converted into a
grayscale image with `PIL`.
Any grayscale value (0..255) below the optional `threshold` argument
is interpreted as black.
"""
image = numpy.array(PIL.Image.open(filename).convert('L')) < threshold
#
# TODO Change this into returning `image` and not converting the
# 2D data into a 1D list with marker elements for the pixel row ends.
#
result = list()
for y, row in enumerate(image, 1):
result.extend([x, y, int(pixel)] for x, pixel in enumerate(row, 1))
result.append([len(row) + 1, y, '\n'])
return result
def get_lines(image):
"""Extract the lines from the image.
The given `image` must be of the form returned by :func:`load_image`
and width and height must be the same or an :exc:`ValueError` will
be raised.
Returns a sequence of lines. Each line is represented as a list with
two pairs of coordinates for the end points of that line.
.. note:: This function alters pixel values of the given `image`.
So if you need that information unchanged be sure to make a copy
of it before passing it to this function.
.. seealso::
:func:`load_image` for a description of how the argument structure
looks like.
"""
#
# TODO Don't restrict this to images with equal width and height without
# a good reason.
#
# TODO This function seems to be too long and too complex (and broken).
#
lines = []
try:
# Length of one pixel row *including* the end marker!
image_row_length = int(len(image)**0.5) + 1
if image[image_row_length - 1][2] != '\n':
raise ValueError('unexpected image row length')
first_one_index = 0
while True:
while True: # Looking for first '1' in image.
if image[first_one_index][2] == 1:
break
first_one_index += 1
start_index = first_one_index
# Looking for next '1'.
end_index = start_index
y = image[start_index][1] - 1
if image[start_index + 1][2] in ['\n', 0]:
# No right line.
if image[start_index + (image_row_length - 1) * y + y][2] == 0:
# No line, just a point.
lines.append([image[start_index], image[end_index]])
image[start_index][2] = 0
else:
# Line down.
try:
while image[end_index + image_row_length][2] == 1:
end_index = end_index + image_row_length
except IndexError:
# Line ends at the very bottom pixel row.
pass
lines.append([image[start_index], image[end_index]])
for i in xrange(
start_index, end_index + 1, image_row_length
):
image[i][2] = 0
elif image[start_index + 1][2] == 1:
# Right line.
while image[end_index + 1][2] == 1:
end_index += 1
lines.append([image[start_index], image[end_index]])
for i in xrange(start_index, end_index + 1):
image[i][2] = 0
else:
#
# The pixel value at `start_index` is not 0, 1, or '\n'
# which should never happen.
#
assert False, str(image[start_index + 1][2])
except IndexError:
# This happens when a non-existant image element is accessed
# which means we have finished. (Does it really mean *that*?)
pass
return lines
def process_file(path):
image = load_image(path)
lines = get_lines(image)
tkMessageBox.showinfo(
title='Bild-Matrix', message=''.join(str(x[2]) for x in image)
)
tkMessageBox.showinfo(title='Linien', message=lines)
def main():
root = tk.Tk()
root.title('Hello Stephan')
tk.Label(root, text='Name of image file:').grid(
row=0, column=0, sticky=tk.E
)
filename_entry = tk.Entry(root)
filename_entry.grid(row=0, column=1, padx=4)
filename_entry.focus_set()
tk.Label(
root,
text=(
'Notice: Presently, image has to be stored\n'
'in the same folder as program'
)
).grid(row=1, column=0, columnspan=2)
tk.Button(
root,
text='Open File',
command=lambda: process_file(filename_entry.get())
).grid(row=0, column=2, pady=4)
tk.Button(root, text='Quit', command=root.quit).grid(
row=1, column=2, pady=4
)
root.mainloop()
if __name__ == '__main__':
main()