[Code-Review]Gleiche Elemente in Excellisten finden

[Dennis89]

Guten Abend,

ich benötige mal wieder eure Hilfe/Meinung/Kritik zu einer Aufgabe die ich mit Python gelöst habe.

Es gibt drei Excel-Listen(dabei handelt es sich um unterschiedliche Baugrößen), in denen befinden sich unter anderem Artikelnummern. Ich will nun die Artikelnummern, die in allen drei Listen vorkommen in eine neue Excel-Datei schreiben und dann will ich noch die Artikelnummern die nur in der Baugröße 70 und 80 verkommen in die Excel-Datei schreiben.

Nun habe ich das zwar geschafft, aber ich tat mir teilweise schwer ordentliche Namen zu finden und das ist für mich immer ein Zeichen, dass ich das zu kompliziert mache. Ich dachte mir, das wenn ich mich aus 'more_itertools' bediene, ein schönerer Code rauskommt, als verschatelte 'for'-Schleifen in denen ich "händisch" über alle Artikel iteriere und vergleiche. Aber irgendwie sieht das Ergebnis für mich so aus, als ob man das noch etwas vereinfachen/eleganter machen könnte

Über jegliche Art von Hinweisen bin ich euch sehr dankbar.

Code: Alles auswählen

from itertools import chain
from pathlib import Path

from more_itertools import duplicates_everseen
from openpyxl import Workbook, load_workbook

PATH_TO_EXCEL = Path(r"C:\Users\Dennis\Documents")
EXCEL_FILES = ["60.xlsx", "70.xlsx", "80.xlsx"]


def create_lists(articles):
    return [
        list(chain(articles[0], articles[1])),
        list(chain(articles[0], articles[2])),
        list(chain(articles[1], articles[2])),
    ]


def read_data_from_excel():
    articles = []
    for file in EXCEL_FILES:
        workbook = load_workbook(filename=Path(PATH_TO_EXCEL, file))
        articles.append(
            [cell.value for cell in workbook["Tabelle1"]["A"] if cell.value]
        )
    return articles


def remove_double_duplicates(articles):
    all_articles = list(chain(articles[0], articles[1]))
    return set(
        [article for article in all_articles if all_articles.count(article) >= 1]
    )


def search_duplicates(articles):
    return [list(duplicates_everseen(article)) for article in articles]


def write_into_excel_file(articles):
    workbook = Workbook()
    worksheet = workbook.active
    for column, article in enumerate(articles):
        for index, item in enumerate(article):
            worksheet.cell(row=index + 1, column=column + 1).value = item
    workbook.save(r"C:\Users\Dennis\Documents\Gleichteile.xlsx")


def main():
    articles = read_data_from_excel()
    lists_to_compare = create_lists(articles)
    duplicates_found = search_duplicates(lists_to_compare)
    duplicates_of_all = list(remove_double_duplicates(duplicates_found))
    write_into_excel_file([duplicates_of_all, duplicates_found[2]])


if __name__ == "__main__":
    main()

Grüße und schönen Abend
Dennis

[__blackjack__]

@Dennis89: Ich habe jetzt nur kurz drüber geschaut, aber da werden nirgends wirklich Mengenoperationen verwendet, was ich hier irgendwie erwartet hätte, denn das sind ja Mengenoperationen.

Und `remove_double_duplicates()` ist sowohl vom Namen her als auch was es macht komisch. Was ist denn der Sinn der „list comprehension“ in der Funktion, ausser Rechenzeit zu verbraten‽

`enumerate()` kann man einen Startwert übergeben, das muss nicht bei 0 anfangen zu zählen.

[Dennis89]

Guten Abend __blackjack__,

da ich drei Listen mit den gefundenen Duplikate habe und diese Listen zu einer zusammenführe, enthält die neue Liste doppelte Duplikate. Mit der 'remove_double_duplicates' will ich dafür sorgen, das jede gefundene Artikelnummer nur einmal in der neuen Liste vorkommt und ich die dann übertragen kann. Vielleicht ist meine Vorgehensweise von Grund auf auch schon etwas umständlich.

Mit Mengenoperationen meinst du, das ich 'set' benutzen soll? Das habe ich zumindest in der 'remove_double_duplicates'.

Danke für deine Antwort.


Grüße und gute Nacht
Dennis

[__blackjack__]

@Dennis89: Die „list comprehension“ macht nichts sinnvolles. Die kopiert eine Liste. Sehr ineffizient wegen der sinnlosen ``if``-Bedingung die für jedes Element noch mal durch die komplette Liste geht, und immer `True` ergibt. Diese `list()`/`chain()`-Kombination würde ich so nicht schreiben, weil ``list(chain(a, b))`` ja nichts anderes als ``[*a, *b]`` ist. Allerdings sollte man mal schauen wo man `list()` überhaupt braucht. Die `remove_double_duplicates()` jedenfalls erstellt zu viele Listen. `set()` benutzt Du schon um Duplikate zu filtern, aber eben keine Mengenoperationen, wie Schnittmenge, Differenzmenge, …. Das sind ja eigentlich die Operationen die da gefragt sind.

Was die Funktion letztlich macht ist vom Effekt her das hier, nur mit unsinniger Laufzeit und Listen mit Zwischenergebnissen:

Code: Alles auswählen

def remove_duplicates(articles):
    return set(chain(articles[0], articles[1]))

Was daran so überhaupt nicht schön ist, sind die magischen Indexwerte.

Ich war mal so frei das `double` aus dem Namen zu nehmen denn „doppelte Duplikate“ ist hier zu viel des guten. Das gibt es ja wirklich, das man manchmal mit doppelten Duplikaten was besonderes machen will, aber mit mehr als Doppelten nicht. Diese Funktion beseitigt aber Duplikate generell.

[__blackjack__]

@Dennis89: Ich habe mir das noch mal angeschaut und finde `articles` auch sehr verwirrend. Das ist eine Liste mit Listen mit einzelnen `article` und immer wenn ich den Namen `articles` lese, weiss ich nicht ist das eine Liste mit Artikeln oder eine Liste mit Listen mit Artikeln.

In `search_duplicates()` ist der Name `article` beispielsweise falsch, denn das ist nicht *ein* Artikel. Ebenso in `write_into_excel_file()` wo über ”einen” `article` iteriert wird — was nur geht, weil es gar nicht `article` heissen dürfte, sondern `articles` heissen müsste. So heisst aber schon die Liste mit den Listen mit Artikeln.

`read_data_from_excel()` ist ein Kandidat für eine „list comprehension“.

`search_duplicates()` gefolgt von `remove_duplicates()` erschliesst sich mir auch nichts so wirklich. Warum ist das nicht in einem Schritt. Warum nicht einfach einen `collections.Counter()` nehmen und da alles raus picken was nur einmal vor kommt?

Ich bekomme das alles nur schwer mit der Beschreibung zusammen.

Hier trotzdem mal leicht überarbeitet mit den Anmerkungen bisher:

Code: Alles auswählen

#!/usr/bin/env python3
from itertools import chain
from pathlib import Path

from more_itertools import duplicates_everseen
from openpyxl import Workbook, load_workbook

PATH_TO_EXCEL = Path(r"C:\Users\Dennis\Documents")
EXCEL_FILENAMES = ["60.xlsx", "70.xlsx", "80.xlsx"]


def read_data_from_excel():
    return [
        [
            cell.value
            for cell in (
                load_workbook(PATH_TO_EXCEL / filename)["Tabelle1"]["A"]
            )
            if cell.value
        ]
        for filename in EXCEL_FILENAMES
    ]


def write_into_excel_file(articles):
    workbook = Workbook()
    worksheet = workbook.active
    for column, article in enumerate(articles, 1):
        for row, item in enumerate(article, 1):
            worksheet.cell(row=row, column=column).value = item
    workbook.save(R"C:\Users\Dennis\Documents\Gleichteile.xlsx")


def create_lists(articles):
    return [
        [*articles[0], *articles[1]],
        [*articles[0], *articles[2]],
        [*articles[1], *articles[2]],
    ]


def remove_duplicates(articles):
    return set(chain(articles[0], articles[1]))


def search_duplicates(articles):
    return [list(duplicates_everseen(article)) for article in articles]


def main():
    articles = read_data_from_excel()
    lists_to_compare = create_lists(articles)
    duplicates_found = search_duplicates(lists_to_compare)
    duplicates_of_all = list(remove_duplicates(duplicates_found))
    write_into_excel_file([duplicates_of_all, duplicates_found[2]])


if __name__ == "__main__":
    main()

Wenn ich die Beschreibung im ersten Beitrag nehme so wie ich den verstehe (was falsch sein kann ), dann käme ich da auf ungefähr das hier (komplett ungetestet):

Code: Alles auswählen

#!/usr/bin/env python3
from pathlib import Path

from openpyxl import Workbook, load_workbook

EXCEL_PATH = Path(r"C:\Users\Dennis\Documents")
EXCEL_FILENAMES = ["60.xlsx", "70.xlsx", "80.xlsx"]


def load_articles(file_path):
    return (
        cell.value
        for cell in load_workbook(file_path)["Tabelle1"]["A"]
        if cell.value
    )


def save_result(file_path, columns):
    workbook = Workbook()
    worksheet = workbook.active
    for column_index, column in enumerate(columns, 1):
        for row_index, value in enumerate(column, 1):
            worksheet.cell(row=row_index, column=column_index).value = value
    workbook.save(file_path)


def main():
    articles_60, articles_70, articles_80 = (
        set(load_articles(EXCEL_PATH / filename))
        for filename in EXCEL_FILENAMES
    )
    common_articles = articles_60 & articles_70 & articles_80
    only_in_70 = articles_70 - articles_60 - articles_80
    only_in_80 = articles_80 - articles_60 - articles_70
    save_result(
        R"C:\Users\Dennis\Documents\Gleichteile.xlsx",
        map(sorted, [common_articles, only_in_70, only_in_80]),
    )


if __name__ == "__main__":
    main()

[Dennis89]

Hallo und vielen Dank für deine Antworten und Erklärungen.

Die fast die meiste Zeit habe ich damit verbracht, alles so gut es geht zu benennen und war dann auch der Grund für meinen Post hier. Wie ich sehe (und vermutet habe) gibt es eine viel einfachere Variante.

Du hast das fast auch richtig verstanden. Ich benötige nur nicht 'only_in_70' und 'only_in_80' sondern die Artikel, die in 70 und 80 vorkommen:

Code: Alles auswählen

in_70_and_80 = articles_70 & articles_80

Die binären Operationen hatte ich absolut gar nicht auf dem Schirm. '&' habe ich soweit auch verstanden (glaube ich), schreibe es aber trotzdem nochmal. Die beiden Werte, vor und nach '&' werden in Binärschreibweise dargestellt und jede Stelle wird verglichen. Bei euer Übereinstimmung schreibt man eine 1 ansonsten eine 0. Bei gleichen Artikelnummern gibt dass dann wieder die Artikelnummer. Gerade beim schreiben fällt mir auf, was passiert mit den anderen? Wieso sind die nicht in der Liste 'common_articles'? Also das ist ja richtig dass die nicht drin sind, aber ich verstehe den Grund dafür nicht. Liegt das an 'set' ? In der Doku dazu habe ich nichs gesehen.
Eigentlich wollte ich fragen was '-' macht? Das konnte ich in der Tabelle hier auch nicht finden.

Vielen Dank und Grüße
Dennis

[__blackjack__]

@Dennis89: Das ``&`` ist ja nur bei ganzen Zahlen ein bitweiser Operator. Bei Mengen ist dass das ”und” aus der Mengenleere. Also A & B enthält alle Elemente die in A *und* B enthalten sind. Gemeinhin auch Schnittmenge genannt. Und ``-`` ist das was man erwarten würde: A - B hat als Ergebnis alle Elemente aus A abzüglich denen die in B sind. Das steht alles bei `set()`.

[Dennis89]

Vielen Danke! Jetzt habe ich das auch bei 'set' gefunden.

Gerade überlege ich, ob es sinnvoll wäre wenn in der Excel-Liste nicht nur die gefundenen Artikelnummern sondern auch die Artikelbeschreibung stehen würde.
Die drei Excel-Dateien die zu Beginn ausgelesen wurden, enthalten auch Artikelbeschreibungen.

Dazu würden mir drei Vorgehensweisen einfallen, entweder ich lese die Beschreibung am Anfang gleich mit aus und erstelle ein Dictonary mit Artikelnummer und Beschreibung oder ich suche bei jedem Schreibvorgang in die neue Excel-Datei den zugehörigen Namen in einer der "alten" Dateien. Ich könnte auch vor dem schreiben in die Excel-Datei ein Dictonary erstellen und dann darin suchen.
Ist von den Varianten eine dabei die du/ihr wählen würdet oder habt ihr eine andere Idee?
Also bitte nur theoretisch in einem oder zwei Sätzen antworten. Den Code dazu würde ich gerne erst selbst schreiben, falls mir noch ein Grund einfällt wieso die Artikelbeschreibung sinnvoll wäre.

Ich habe jetzt noch gar nicht nachgeschaut, ob und wie die Ideen umsetzbar sind, das fiel mir nur so wärend des schreibens ein und selbst wenn ich das nicht benötige, wäre dass meiner Mienung nach eine interessante Codeerweiterung, zumindest für mich zum lernen.

Viele Grüße
Dennis

[Sirius3]

Vorgehensweise 1 ist das, was man üblicherweise macht.

[Dennis89]

Danke, ich melde mich dann wieder mit Codezeilen zurück.

Grüße
Dennis

[Dennis89]

Hallo und schönen Sonntag zusammen,

habe meine erste Version mal fertig gestellt. Erste Tests brachten das gewünschte Ergebnis, das ist schon mal gut.
Mir gefällt nicht, das ich drei Dictonarys erstelle, dann jedes einzelne mit 'set' umwandle und am Schluss nur eins benötige um die Artikelbeschreibung rauszuholen. Mir würde eigentlich ein Dictonary reichen, aber das wäre dann wieder zusätzlicher Code, der vielleicht auch nicht gleich verständlich ist.
Benutzt man '__setitems__' so wie ich es gemacht habe oder hätte man das mit 'update' gemacht oder würde man hier ganz anders (sinnvoller/eleganter) vorgehen?
In 'save_results' musste ich auf das 'enumerate' für den Spaltenindex verzichten, da ich mir sonst meine "erste" Artikelbeschreibung mit den Artikelnummern aus 'common_70_80' überschreiben würde.

Bin wieder über eure Meinung gespannt:

Code: Alles auswählen

#!/usr/bin/env python3
from pathlib import Path

from openpyxl import Workbook, load_workbook

EXCEL_PATH = Path(r"C:\Users\Dennis\Documents")
EXCEL_FILENAMES = ["60.xlsx", "70.xlsx", "80.xlsx"]


def load_articles(file_path):
    articles = {}
    for row, cell in enumerate(load_workbook(file_path)["Tabelle1"]["A"], 1):
        if cell.value:
            articles.__setitem__(
                cell.value, load_workbook(file_path)["Tabelle1"][f"B{row}"].value
            )
    return articles


def save_result(file_path, columns, articles):
    workbook = Workbook()
    worksheet = workbook.active
    column_index = 1
    for column in columns:
        for row_index, value in enumerate(column, 1):
            worksheet.cell(row=row_index, column=column_index).value = value
            worksheet.cell(row=row_index, column=column_index + 1).value = articles[value]
        column_index += 2
    workbook.save(file_path)


def main():
    articles_60, articles_70, articles_80 = (
        load_articles(EXCEL_PATH / filename) for filename in EXCEL_FILENAMES
    )
    common_articles = set(articles_60) & set(articles_70) & set(articles_80)
    common_70_and_80 = set(articles_70) & set(articles_80)
    save_result(
        r"C:\Users\Dennis\Documents\Gleichteile.xlsx",
        map(sorted, [common_articles, common_70_and_80]),
        articles_80,
    )


if __name__ == "__main__":
    main()

Dankeschön und Grüße
Dennis

[__blackjack__]

@Dennis89: `__setitem__` würde man so nicht aufrufen, und natürlich auch nicht `update()` sondern einfach ganz normal ``a_dict[key] = value``.

In der Funktion wird für jeden Wert `load_workbook()` aufgerufen, also für jeden Wert in der Tabelle die gleiche Tabelle noch mal geladen. Die `Sheet`-API bietet zudem passende Methoden das man nicht über eine Spalte iterieren muss und sich den Wert der zweiten Spalte in der gleichen Zeile dann irgendwie anders besorgen muss. Man kann gleich über die passenden Paare iterieren:

Code: Alles auswählen

def load_articles(file_path):
    sheet = load_workbook(file_path)["Tabelle1"]
    articles = {}
    for article_cell, description_cell in sheet.iter_rows(
        min_col=1, max_col=2
    ):
        if article_cell.value:
            articles[article_cell.value] = description_cell.value
    return articles

Und das ist jetzt einfach genug um daraus eine „dict comprehension“ zu machen.

Auch das schreiben beziehungsweise erstellen der Tabelle geht einfacher, ohne das man da mit Zeilen- und Spaltenindizes arbeiten muss.

Ungetestet:

Code: Alles auswählen

#!/usr/bin/env python3
from pathlib import Path

from openpyxl import Workbook, load_workbook

EXCEL_PATH = Path(r"C:\Users\Dennis\Documents")
EXCEL_FILENAMES = ["60.xlsx", "70.xlsx", "80.xlsx"]


def load_articles(file_path):
    worksheet = load_workbook(file_path)["Tabelle1"]
    return {
        article_cell.value: description_cell.value
        for article_cell, description_cell in worksheet.iter_rows(
            min_col=1, max_col=2
        )
        if article_cell.value
    }


def save_result(file_path, columns, articles):
    workbook = Workbook()
    worksheet = workbook.active
    for row_values in zip(*columns):
        row = []
        for value in row_values:
            row.extend([value, articles[value]])
        worksheet.append(row)

    workbook.save(file_path)


def main():
    articles_60, articles_70, articles_80 = (
        load_articles(EXCEL_PATH / filename) for filename in EXCEL_FILENAMES
    )
    common_70_and_80 = set(articles_70) & set(articles_80)
    common_articles = set(articles_60) & common_70_and_80
    save_result(
        R"C:\Users\Dennis\Documents\Gleichteile.xlsx",
        map(sorted, [common_articles, common_70_and_80]),
        articles_80,
    )


if __name__ == "__main__":
    main()

[Dennis89]

Vielen Dank für deine Antwort und deinen Verbesserungen.

Das war ja mal wieder unnötig kompliziert von mir.
Allerdings suche ich gerade noch einen Alternative für ‚zip‘, denn es kann ja vor kommen, dass die Listen nicht gleich lang sind und dann gehen mir Artikel „verloren“.

Bis jetzt bin noch nicht fündig geworden. Muss ich das händisch machen oder gibt es dafür auch was ?

Grüße
Dennis

[__blackjack__]

@Dennis89: `itertools.zip_longest()` könnte helfen.

[Dennis89]

Danke, das funktioniert. Allerdings werden die "Leerstellen" mit 'None' gefüllt, das endet dann in einem Fehler mit dem Dictonary. Ich habe jetzt noch eine 'if'-Abfrage eingebaut, die dafür sorgt das bei 'None' keine Werte verarbeitet werden.

Code: Alles auswählen

def save_result(file_path, columns, articles):
    workbook = Workbook()
    worksheet = workbook.active
    for row_values in zip_longest(*columns):
        row = []
        for value in row_values:
            if value is not None:
                row.extend([value, articles[value]])
        worksheet.append(row)

    workbook.save(file_path)

Siehst du da noch irgendwelche Probleme oder kann ich das so lassen?
Ein Test mit wenigen Testdaten war zumindest mal erfolgreich.

Vielen Dank und Grüße
Dennis

Edit:
Besser wäre es vielleicht so:

Code: Alles auswählen

def save_result(file_path, columns, articles):
    workbook = Workbook()
    worksheet = workbook.active
    for row_values in zip_longest(*columns):
        row = []
        for value in row_values:
            if value is not None:
                row.extend([value, articles[value]])
            else:
                row.extend(([None, None]))
        print(row)
        worksheet.append(row)

    workbook.save(file_path)

Ansonsten "rutschen" mir die Artikel vom zweiten Vergleich in die erste Spalte. Das wäre hier zwar kein Problem, wenn man es weis, aber schön und selbsterklärend ist es ja auch nicht.

[narpfel]

@Dennis89: Wenn unterschiedliche Längen ein Fehler sind und nicht einfach ignoriert werden sollen, könntest du anstatt `zip_longest` vielleicht das normale `zip` mit `strict=True` benutzen. Das wirft eine Exception, wenn nicht alle Iterables gleich lang sind. Achtung: Das ist neu in Python 3.10.

Und zu deiner Frage zu `__setitem__`: Generell gilt für alle „dunder“-Namen: Die benutzt man im Normalfall nie direkt, sondern immer den dazugehörigen Operator. Also `x + y` statt `x.__add__(y)`, `len(xs)` statt `xs.__len__()`, `a.b = c` statt `a.__setattr__("b", c)`, `type(foo)` statt `foo.__class__`, etc.

[Dennis89]

Hallo,

danke für die Erklärung.

Unterschiedliche Längen sind kein Fehler, es wäre eher ein Fehler wenn man eine Liste kürzen würde. Aber trotzdem Danke für die Info, vielleicht kann ich das mal gebrauchen.

Grüße
Dennis

Achja, ich habe heute den Code mit den dafür vorgesehenen Daten getestet und es hat einwandfrei funktioniert. An der Stelle nochmal Danke für die Unterstützung.