Einfache Verbesserungsvorschläge erwünscht, CSV-Projekt

Stellt hier eure Projekte vor.
Internetseiten, Skripte, und alles andere bzgl. Python.
Antworten
ravenheart
User
Beiträge: 70
Registriert: Mittwoch 10. November 2010, 18:41

Manche wissen vielleicht schon, dass ich seit einiger Zeit versuche für csv-ähnliche Dateien ein paar Funktionen zu schreiben,
Vor allem brauche ich 3 Funktionen.

select_columns( Spaltennamen )
select_rows( Bedingung)
uniq ( Spaltennamen) // soll alle Kombinationen von Spalten ausgeben

Ich habe hier meine erste rein sequentielle Implementierung soweit fertig und würde gerne nette Kritik hören
( ich lerne python eben erst und weiß dass weder Design noch Implementierung perfekt sind, deswegen bitte keinen Herzinfarkt bekommen und nett bleiben ^^)

Das ganze lässt sich folgedermaßen bedienen:

Code: Alles auswählen

from pm_utils import *

pmd = pmd_handler( Dateipfad )
pmd.select_columns("x,y,z").select_rows("time > 10 and mode =='speed'").dump( neuerName )
genug gequasselt:

Code: Alles auswählen

# -*- coding: utf-8 -*-
import os
import re
import ast
import tempfile

pattern_number_of_cols = re.compile( "^# cols=" )
pattern_names_of_cols  = re.compile( "^# [\w \t]*")
pattern_simple_comment = re.compile( "^#" )
pattern_number         = re.compile( "^[-+]?[0-9]*\.?[0-9]+([eE][-+]?[0-9]+)?$" )

use_separator = "\t"


class pmd_handler:

    def __init__(self, pmd = None):

        self.col_names           = None
        self.number_of_cols      = None
        self.cols                = None
        self.map_name_id         = None
        self.pmd                 = None


        if pmd != None:
            self.set_pmd(pmd)    

    def set_pmd(self, path):
        
        if os.path.isfile(path):
            file = open(path)

            number_of_cols = None
            names_of_cols  = None

            for line in file:
                if number_of_cols == None and pattern_number_of_cols.match(line):
                    number_of_cols = int( line[7:].strip()  ) 
                if names_of_cols == None and pattern_names_of_cols.match(line) and not pattern_number_of_cols.match(line):
                    names_of_cols = [ col.strip() for col in line[1:].split() ]

            file.close()

            if number_of_cols == None or names_of_cols == None:
                raise ValueError("Keine gültige pmd-Datei")

            if len(names_of_cols) != number_of_cols:
                raise ValueError("Keine gültige pmd-Datei")

            cols = []
            for i in range(number_of_cols):
                cols.append( (i, names_of_cols[i]))
            cols = tuple(cols)

            map_name_id = {}
            for k in sorted(dict(cols).keys()):
                map_name_id[dict(cols)[k]] = k

            self.col_names          = names_of_cols
            self.number_of_cols     = number_of_cols
            self.cols               = cols
            self.map_name_id        = map_name_id
            self.pmd                = path
    
    def _enter(self):
        file = open(self.pmd)
        return file

    def _exit(self,file):
        file.close()
        

    def uniq( self,  colnames ):
        uniq = []

        if isinstance(colnames, str):
            colnames = colnames.split(",")
            colnames = [ x.strip() for x in colnames]
        
        indices = []
        for name in colnames:
            indices.append( self.map_name_id[name] )

        file = self._enter()
        for line in file:
            if pattern_simple_comment.match(line):
                continue
            current = []
            line = line.split()
            for i in indices:
                current.append( line[i] )
            if current not in uniq:
                uniq.append(current)
        
        self._exit(file)

        return uniq

    def select_columns(self, colnames):
        
        file = self._enter()
        
        if isinstance(colnames, str):
            colnames = colnames.split(",")
            colnames = [ x.strip() for x in colnames]
        
        header_number_of_cols = "# cols= {0}".format(len(colnames))
        header_names_of_cols  = "# "
        for name in colnames:
            header_names_of_cols = header_names_of_cols + name + use_separator

        tmp = tempfile.TemporaryFile()
        tmp.write( header_number_of_cols + "\n")
        tmp.write( header_names_of_cols  + "\n")

        indices = []
        for name in colnames:
            indices.append( self.map_name_id[name] )

        for line in file:
            if pattern_simple_comment.match(line):
                continue
            
            line = line.split()
            for i in indices:
                tmp.write( line[i] + use_separator )
            tmp.write("\n")

        self._exit(file)
        
        handler = temporary_pmd_handler(tmp)
        return handler
    
    def select_rows(self, condition):
        indices = []
        var = []

        expression = ast.parse(condition, mode="eval")
        class Transformer(ast.NodeTransformer):
            def visit_Name(self,node):
                indices.append(node.id)
                return ast.copy_location(ast.Subscript( value =ast.Name(id="var", ctx=ast.Load()), slice = ast.Index( value= ast.Num(len(indices)-1)), ctx = node.ctx), node)
        Transformer().visit(expression)
        expression = ast.fix_missing_locations(expression)
        code = compile(expression, "<string>", mode="eval")
        indices = [ self.map_name_id[x] for x in indices]
       
        
        #class v(ast.NodeVisitor):
        #    def generic_visit(self,node):
        #        print type(node).__name__
        #        ast.NodeVisitor.generic_visit(self,node)
        #    def visit_Name(self,node):
        #        print "Name: ", node.id
        #    def visit_Num(self,node):
        #        print "Num: ", node.n
        #v().visit(expression)

        file = self._enter()
        tmp = tempfile.TemporaryFile()

        for line in file:
            if pattern_number_of_cols.match(line):
                tmp.write(line)
            if pattern_names_of_cols.match(line) and not pattern_number_of_cols.match(line):
                tmp.write(line)

        file.seek(0)
        for line in file:
            if not pattern_simple_comment.match(line):
                splitline = line.split()
                var = []
                x = []
                for i in indices:
                    x.append(splitline[i])
                for item in x:
                    if pattern_number.match(item):
                        var.append(float(item))
                    else:
                        var.append(item)
                
                if eval(code) == True:
                    tmp.write(line)
        
        self._exit(file)

        handler = temporary_pmd_handler(tmp)
        return handler




    def dump(self, path):
        file = self._enter()
        out = open(path, "w")
        for line in file:
            out.write(line)
        out.close()
        self._exit(file)
        return pmd_handler(path)


class temporary_pmd_handler(pmd_handler):

    def __init__(self, tmpfile= None ):
        self.col_names = None
        self.cols = None
        self.map_name_id = None
        self.number_of_cols = None
        
        if tmpfile != None:
	        self.set_pmd(tmpfile)

    def set_pmd(self, tmpfile):
        
        file = tmpfile
        file.seek(0)

        number_of_cols = None
        names_of_cols  = None

        for line in file:
            if number_of_cols == None and pattern_number_of_cols.match(line):
                number_of_cols = int( line[7:].strip()  ) 
            if names_of_cols == None and pattern_names_of_cols.match(line) and not pattern_number_of_cols.match(line):
                names_of_cols = [ col.strip() for col in line[1:].split() ]


        if number_of_cols == None or names_of_cols == None:
            raise ValueError("Keine gültige pmd-Datei")

        if len(names_of_cols) != number_of_cols:
            raise ValueError("Keine gültige pmd-Datei")

        cols = []
        for i in range(number_of_cols):
            cols.append( (i, names_of_cols[i]))
        cols = tuple(cols)

        map_name_id = {}
        for k in sorted(dict(cols).keys()):
            map_name_id[dict(cols)[k]] = k

        self.col_names          = names_of_cols
        self.number_of_cols     = number_of_cols
        self.cols               = cols
        self.map_name_id        = map_name_id
        self.pmd                = tmpfile

    def _enter(self):
        self.pmd.seek(0)
        return self.pmd

    def _exit(self, file):
        pass






""" Nicht in KLasse """
def is_valid_pmd_file(file):
    
    if not os.path.isfile(file):
        return False

    file = open(file)

    number_of_cols = None
    names_of_cols  = None
        
    for line in file:
        if number_of_cols == None and pattern_number_of_cols.match(line):
            number_of_cols = int( line[7:].strip()  ) 
        if names_of_cols == None and pattern_names_of_cols.match(line) and not pattern_number_of_cols.match(line):
            names_of_cols = [ col.strip() for col in line[1:].split() ]
        
    file.close()

    if number_of_cols == None or names_of_cols == None:
        return False

    if len(names_of_cols) != number_of_cols:
        return Falsie
        
    return True
Wenn ich soweit bin und etwas Zeit habe, werde ich den Ansatz den BlackJack mir geliefert hat etwas intensiver verfolgen
(Es werden noch einige Fragen dazu kommen ^^)
Aber im Moment werde ich wohl damit arbeiten
BlackJack

@ravenheart: Fangen wir mit dem Wichtigsten an: Es gibt Zeilen die länger als 80 Zeichen sind. :-)

Die Namensgebung entspricht nicht dem Python Style Guide (PEP8). Konstanten sollten in GROSSBUCHSTABEN_MIT_UNTERSTRICHEN benannt werden und Klassennamen in MixedCase.

Leerzeichen vor und nach Operatoren und Zuweisungen -- ausgenommen Zuweisungen in Argumentlisten bei der Definition oder beim Aufruf. Keine Leerzeichen nach und vor Klammern bei Funktionsaufrufen. Kein Ausrichten von ``=``. Leerzeichen nach Kommas.

`file` ist der Name eines eingebauten Typs, diese Namen sollte man nach Möglichkeit nicht für anderes verwenden.

Namenszusätze bei Klassen wie `Handler` oder `Manager` finde ich persönlich nicht so toll. Wenn einem nichts vernüftiges einfällt, kann man auch einfach gar keinen Zusatz verwenden. :-)

Ist `pmd_handler.number_of_cols` nicht redudant? Das sollte doch immer der Länge von `col_names` entsprechen, oder?

Bei `None` kann man auf Objektidentität testen, weil die Sprache garantiert, dass es nur ein einziges solches Objekt geben kann.

Tests auf die Existenz von Dateien oder ob es tatsächlich eine Datei ist, kann man sich sparen. Einfach öffnen und eben auf eine Ausnahme reagieren. Das muss man ja sowieso, weil selbst eine existierende Datei zwischen dem Test und dem Öffnen, von der Platte gelöscht werden kann. Dateien öffnen dann mit der ``with``-Anweisung. Ähnliches gilt für die Testfunktion: Wenn es keine gültige PMD-Datei ist, dann löst Deine Klasse ja eine Ausnahme aus -- wozu dann noch eine zusätzliche Testfunktion!?

Beim `pmd_handler` gefällt mir das mit dem `set_pmd()` nicht so gut, beziehungsweise dass man so eine Klasse erstellen kann die nicht "benutzbar" ist, weil alle Datenattribute an `None` gebunden sind. Damit haben Exemplare zwei sehr unterschiedliche Zustände. Ein Exemplar ohne zugehörige Datei erscheint mir sinnlos.

Was Du da in `set_pmd()` mit `cols` machst, ist *sehr* verwirrend. Erst erzeugst Du eine Liste mit Tupeln (index, element), dann machst Du aus der Liste ein Tupel. Daraus erzeugst Du ein Dictionary nur um die Schlüssel heraus zu holen und zu sortieren, wobei das was da heraus kommt nichts anderes als ``range(number_of_cols)`` ist. Und in der Schleife erzeugst Du bei *jedem* Durchlauf ein Dictionary mit dem *gleichen Inhalt* nur um dann auf *einen* Wert darin zuzugreifen. Argh! Das ist im Grunde ein Einzeiler:

Code: Alles auswählen

            map_name_id = dict((n, i) for i, n in enumerate(names_of_cols))
Das Filtern von Kommentarzeilen wird ja öfter gebraucht, das würde ich in eine eigene Generatorfunktion auslagern. Und persönlich finde ich ``continue`` nicht so schön, weil das ja so eine Art ``GOTO`` ist.

In `uniq()` könnte man von `operator.itemgetter()` gebrauch machen und der ``in``-Test auf einer Liste ist eine ganz furchtbare Idee was die Laufzeit angeht. Hier sollte man zusätzlich ein `set()` mit den bereits "gesehenen" Daten verwalten.

Die beiden `set_pmd()`-Methoden enthalten eine Menge duplizierten Quelltext!? Kann man nicht eine Klasse schreiben die beide Fälle abdeckt!?

Zeichenketten sind keine Kommetare, ausser sie stehen *nach* einer ``class`` oder ``def`` Zeile, oder am Anfang des Moduls als erster "nicht-Kommentar". Dann sind es "Docstrings", die als Attribut auf dem entsprechenden Objekt gespeichert werden und mit `help()` abgerufen oder entsprechenden Werkzeugen aus dem Quelltext extrahiert werden können um API-Dokumentation zu erstellen.
EyDu
User
Beiträge: 4881
Registriert: Donnerstag 20. Juli 2006, 23:06
Wohnort: Berlin

Hallo.

Ich liste mal alles auf, was mir von oben nach unten aufgefallen ist. Wenn man ins Detail geht, findet man sicher noch mehr.

- An den rechten und linken Rander der Parameterliste bei Aufrufen kommen keine Leerzeichen

Code: Alles auswählen

def function(param1, param2, param3): #so soll es nicht sein
def function(param1, param2, param3): #so ist es richtig
- Konstanten werden durchgängig in Großbuchstaben geschrieben
- Gleiheitszeichen gehören nicht auf eine Ebene gerückt (hatte ich das nicht schon einmal geschrieben?)
- Klassenamen werden in CamelCase geschrieben
- keine Leerzeichen bei Zuweisungen von Defaultwerten bei Parametern
- verwende besser "if pmd is not None" an Stelle von "if pmd != None"
- "file" ist kein guter Name, da du damit einen eingebauten Typen verdeckst
- zum Öffnen und Schließen von Dateien benutze das with-Konstrukt
- Warum testest du, ob es sich um eine Datei handelt? Am besten behandelst du nur die Exception, falls eine geworfen wird (EAFP vs LBYL)
- Warum fasst du das werfen der Exceptions nicht zusammen?
- Wenn du Indizes zu Elementen einer Liste möchtest, dann benutze die enumerate-Funktion.

Code: Alles auswählen

cols = tuple(enumerate(names_of_cols))
- Warum erstellst du das Dictionary aus "cols" immer wieder in der Schleife?
- Du möchstes außerdem unbedingt die zip-Funktion kennenlernen ;-)
- "_enter" und "_exit" sind in dieser Form vollkommen überflüssig.
- Du möchtest das Ergebnis von open nicht noch an einen Namen binden und das Ergebnis dann zurückgeben.
- "uniq" ist ein seltsamer Name.
- "indices" kann man auch locker in einer List Comprehension schreiben.
- Du möchhtest dir noch immer unbedingt das with-Statement anschauen.
- Die Liste "current" kann man mit einer LC erstellen
- Eine Namen innerhalb einer Funktion zu erzeugen, welcher genau so lautet wie die Funktion selbst, ist nicht besonders hübsch.
- der Anfang von "uniq" und "select_columns" sind identisch, das kann man zusammenfassen.
- temporäre Dateien muss man wieder schließen und löschen.
- "indices" wieder als LC
- du musst nicht extra einen Namen "handler" erzeugen, um den Wert zurückzugeben
- eine Klasse innerhalb einer Funktion zu erzeugen kann durchaus nützlich sein, aber in "select_rows" solltest du das nicht tun
- die Bedingungen der ersten for-Schleife waren doch oben fast genau so schon einmal zu sehen...
- "x" ist ganz schön nichtssagend. Kann man außerdem mit einer LC erzeugen
- Du solltest hier noch zusammenfassen:

Code: Alles auswählen

if pattern_number.match(item):
    var.append(float(item))
else:
    var.append(item)
- das "== True kannst du dir sparen
- das "handler" kannst du dir natürilch auch wieder sparen
- "dump": Ich erwähne zur Sicherheit noch einmal das with-Statement ;-)
- Dateiobjete haben eine writelines-Methode
- "set_pmd" stand oben fast genau so schon einmal da
- das selbe in "is_valid_pmd_file"; wenn du Code kopierst, dann willst du Funktionen benutzen

Das mal so auf die schnelle. Da ist sicher noch einges mehr drin und ich habe mich nicht einmal die Struktur angeschaut.

Sebastian

Edit: Da brauchen wir wohl einen Button, wer gerade am "zerpflücken" welches Quelltextes ist ^^
Das Leben ist wie ein Tennisball.
ravenheart
User
Beiträge: 70
Registriert: Mittwoch 10. November 2010, 18:41

- "_enter" und "_exit" sind in dieser Form vollkommen überflüssig.
Hmm, da mein temporärer Handler eine Tempfile hat und keine eigentliche Datei, brauche ich da kein Öffnen.
Im Gegenteil, wenn ich das versuche aufzurufen, erhalte ich Exceptions. Ich könnte das in einen try-catch Block hauen, aber das widerspricht meinem Geschmack, der vielleciht streitbar ist.
Was ich sagen will, diese Funktionen sind deswegen da, weil ich nicht alle Funktionen neu schreiben wollte, mit dem unterschied, dass einmal ein open dabei ist und einmal nicht.
Für eine bessere Lösung wäre ich dankbar.
map_name_id = dict((n, i) for i, n in enumerate(names_of_cols))
habe ich zuerst in BlackJacks Code gesehen. Finde ich sehr nützlich, also das kannte ich bisher noch nicht einmal. (Bei enumerate denk ich immer automatisch an was anderes)
Die Namensgebung entspricht nicht dem Python Style Guide (PEP8). Konstanten sollten in GROSSBUCHSTABEN_MIT_UNTERSTRICHEN benannt werden und Klassennamen in MixedCase.
Ich weiß^^, leider lese ich PEP8 nur nebenbei etwas, auch das mit dem Zurechtrücken von Zuweisungen habe ich gelesen (wobei ich den Sinn davon nicht verstehe, es sieht doch schöner aus). Ich werde, sobald ich erstmal was funktionierendes zusammen gehackt habe, mich darum kümmern.
Auch finde ich was ich bei Blackjack gefunden habe die Bezeichnung x2y für dicts besser. Da ich bisher nur C und Java geschrieben habe, sind das eben maps für mich.
Ich war auch sehr überrascht als ich die Funktion map() gefunden habe und diese nicht tat was ich dachte.
Du möchhtest dir noch immer unbedingt das with-Statement anschauen.
Ich kenne das with-statement.
Zu meiner Beschämung muss ich zugeben, dass ich nicht verstehe WIE es (genau) funktioniert. Ich könnte es aber durchaus anwenden.
Ich weiß aber nicht, wo genau der Vorteil liegt. Vielleicht bin ich da etwas c-altmodisch. Aber ich mag so etwas wie open <-> close
(Auch wenn ich mich bisher nicht darum kümmere, was im Exceptionfall passiert ... Vielleicht ist das der große Vorteil, dass ich kein finally bräuchte...)
"uniq" ist ein seltsamer Name.
Der kommt nicht von mir, so heißt das Skript, das die selbe Funktionalität bietet.
"x" ist ganz schön nichtssagend. Kann man außerdem mit einer LC erzeugen
Jupp, ich wollte am Anfang etwas schreiben wie

Code: Alles auswählen

 var = [float(x) for x in var if pattern_number.match(x)] 
Leider bringe ich da kein else unter so dass andere Elemente auch geaddet werden.
Warum erstellst du das Dictionary aus "cols" immer wieder in der Schleife?
Bin ich blind? finde es nicht ^^
- temporäre Dateien muss man wieder schließen und löschen.
Hmm, das mit dem Schließen kann ja sein, aber löschen? Ich dachte das wird automatisch übernommen? Schließlich sind es ja tempfiles. Werd ich nochmal nachsehen.
Zum Schließen: Im Moment verlasse ich mich darauf, dass das ganze vom GC übernommen wird.
Da meine temporären Objekte ja etwas länger bestehen müssen, wüsste ich spontan nicht wo ich ein Schließen einarbeiten sollen, danach wirds ja auch schwierig das ganze wieder zu öffnen.

operator.itemgetter()
Kannte ich bis morgen nicht
Namenszusätze bei Klassen wie `Handler`
Auch nicht meine Idee =)


Wie gesagt, ich lerne noch.
Ich muss zugeben, dass es etwas schwierig ist, sich unter dem Druck etwas produzieren zu müssen in die Materie einzuarbeiten.
Ich stolpere mehr oder weniger immer wieder über nützliche Sachen, die ich brauche oder brauchen könnte.
Dennoch muss das ganze in erster Linie erstmal funktionieren. Um Style oder ordentliche Fehlerbehandlung oder Optmierungen, welche mehr Zeit in Anspruch nehmen würden kann ich mich erst im Anschluss kümmern.
Persönlich ist mir noch etwas aufgefallen. In select_rows sollte ich nicht alle namen ersetzen, damit noch vergleiche mit Variablen möglich sind. Ausserdem sollte ich noch abfragen, ob eine automatische Umwandlung in float erwünscht ist

So, es gibt noch einiges zu tun, auch in diesem Miniprojekt.
Ich werde demnächst plot-funktionen hinzufügen. Dazu verwende ich wahrscheinlich das gnuplot-paket. (da im Skript auch gnuplot genommen wird)

Ich danke allen bisherigen Kommentatoren und eventuell folgenden.
BlackJack

@ravenheart: Das mit `_enter()` und `_exit()` könntest Du vielleicht loswerden in dem Du die API von den beiden Klassen vereinheitlichst, zum Beispiel das beide eine geöffnete Datei als Argument entgegennehmen. Und dann brauchst Du vielleicht auch nicht mehr zwei verschiedene Klassen!? Der "ersten" übergibst Du die offene Originaldatei und die, die von den Methoden erzeugt werden, geben dann die jeweilige temporäre Datei weiter.

Mit dem Dictionary in der Schleife meinte EyDu wahrscheinlich `names_of_cols` und nicht `cols`. Selbst wenn Du da nicht den Einzeiler und `enumerate()` oder `zip()` verwendest, wäre eine deutlich einfachere Lösung näher liegender gewesen. Zum Beispiel C- oder Java-ähnlich einfach mit einer Zählschleife:

Code: Alles auswählen

map_name_id = dict()
for index in xrange(len(names_of_cols)):
    map_name_id[names_of_cols[i]] = i
Die ``with``-Anweisung funktioniert mit Objekten die eine `__enter__()`- und eine `__exit__()`-Methode besitzen. `__enter__()` wird am Anfang aufgerufen und `__exit__()` beim Verlassen des Blocks -- und zwar auch wenn das durch eine Anweisung oder Ausnahme mitten im Block passiert. Bei Dateiobjekten macht `__enter__()` nichts und `__exit__()` schliesst die Datei. Ein anderes Beispiel für den Einsatz wären zum Beispiel Locks, die beim `__enter__()` sperren und beim `__exit__()` entsperren.

Bei der LC für die Zahlen-Umwandlung falls möglich, könntest Du einen bedingten Ausdruck verwenden:

Code: Alles auswählen

var = [(float(x) if pattern_number.match(x) else x) for x in var]
Ich hätte das aber eher in eine Funktion gesteckt und statt einer `re` einfach versucht `float()` anzuwenden. Wenn das einen `ValueError` auslöst, dann war's halt keine Darstellung einer Fliesskommazahl.

`TempFile`\s werden gelöscht, wenn sie geschlossen werden.
Antworten