Refaktoring

[nmilosev]

TODO:

• [x] Pomeriti sve testove u tests folder, da mogu da budu pokrenuti sa pytest-om
• [x] Main fajl je prevelik, pomeriti deo za parsiranje argumenata u odvojeni fajl (ili čak bolje fajlove)
• [x] Na par mesta imamo prazan exception blok, proveriti da li je to u redu (npr. embedding.py:215)
• [x] Na dosta mesta imamo hvatanje generičkog izuzetka
• [x] Imena promenljivih nisu uvek konzistentnog formata
• [x] Na par mesta imamo unreachable kod (uglavnom nedovršene implementacije, npr load_csv)
• [x] Promeniti imena promenljivih koja su rezervisane reči (npr. map i eval)
• [x] Metodi hub_eval i estimate_lids_bfs, get_map (u unsupervised_link_prediction.py) imaju preveliku kognitivnu složenost (razbiti na manje metode, smanjiti ugnježdenost petlji sa early quit strategijom)
• [x] U fajlu lideval.py treba promeniti imena klasa
• [x] Skloniti sve unused variables (ima dosta)
• [x] Skloniti zakomentarisan kod ili ga pomeriti u docstringove
• [ ] Dodati nedostajuće docstringove makar za module, ako nema vremena da dodajemo za sve funkcije
• [ ] Pogledati promenljive sa kratkim imenima (w, x i slično) i zameniti ih gde je potrebno sa razumljivijim imenima
• [x] Obrisati nekorišćene importe u projektu (pylint W0611)
• [x] Na puno mesta imamo C0301 Line too long ali to ne vredi dirati jer koristimo Black, samo da ostavimo
• [x] Ima dupli kod u testovima, to je u redu, dosta se ponavlja (možda napraviti generičku funkciju za testiranje novog embedding metoda)

Komentari od @milsav:

• [x] svi moduli za evaluaciju (osim za racunanje graph reconstruction metrika koje su trenutno dobro pozicionirane) mislim da treba da zavrse u jednom folderu, recimo u onom evaluation, tako da onda tu treba prebaciti modul za LID-based evaluaciju (lideval) iz common, kao i ceo classifications folder
• [x] u modulu lideval zamolicu da ne menjate nista (ili ako cete nesto da menjate onda dobro istestirajte modul nakon izmena tako da smo sigurni da se tu nista nije promenilo glede realizovanih funkcionalnosti)
• [x] razmotriti da li cog. complexity komentari imaju smisla
• [x] u classifications postoje code clones, tu se svi moduli osim neural_network.py razlikuju u jednoj liniji koda, cini mi se da u nekim od tih modula nedostaju i konstruktori. Stvar je ovde jednostavna: sve scikit-learn klasifikatore treba izvuci iz iste bazne klase (koja nasledjuje jos bazniji Classifier) u kojoj se desava fit i predict, a racunanje accuracy, precision i recall treba izmestiti iz tih klasa u zaseban modul tako da se kod za evaluaciju klasifikatora ne kopira po klasifikator modulima, te da sutra mogu da se dodaju drugi moduli koji realizuju naprednije tehnike za evaluaciju klasifikatora (npr. nested K-fold cross-validation).
• [x] u konstruktoru za GCNEmbedding parametar add_self_loop=False mi se cini suvisnim, u samom konstruktoru je moguce napraviti jednostavnu proveru da li treba dodavati self-loops ili ne, a sa tim parametrom GCNEmbedding tesko ide u embfactory. Alternativa je da se u dataset_pool napravi metod koji za dataset vraca da li treba dodavati self-loops ili ne i onda nakon te izmene izmeniti i postojeci embfactory. Mislim da je prvi pristup jednostavniji i cistiji.

[nmilosev]

Urađen je veliki refaktoring. Imam par komentara na gornje TODO stavke:

Metodi hub_eval i estimate_lids_bfs, get_map (u unsupervised_link_prediction.py) imaju preveliku kognitivnu složenost (razbiti na manje metode, smanjiti ugnježdenost petlji sa early quit strategijom)

Ovo nisam dirao, mislim da je @milsav u pravu što se tiče ovih specifičnih metoda. Ako mislite da ima potrebe da se "razlože" molio bih onda da to uradi ko je pisao kod.

Main fajl je prevelik, pomeriti deo za parsiranje argumenata u odvojeni fajl (ili čak bolje fajlove)

Izmešteno je sve u jedan fajl za parsiranje, sa odvojenim funkcijama za embeding metode, meni se to čini kao ok rešenje. Ne bih odvajao svaki u poseban fajl.

u modulu lideval zamolicu da ne menjate nista (ili ako cete nesto da menjate onda dobro istestirajte modul nakon izmena tako da smo sigurni da se tu nista nije promenilo glede realizovanih funkcionalnosti)

Ovde nisam ništa dirao osim imena klasa da budu formatirana na standardni način

u classifications postoje code clones, tu se svi moduli osim neural_network.py razlikuju u jednoj liniji koda, cini mi se da u nekim od tih modula nedostaju i konstruktori. Stvar je ovde jednostavna: sve scikit-learn klasifikatore treba izvuci iz iste bazne klase (koja nasledjuje jos bazniji Classifier) u kojoj se desava fit i predict, a racunanje accuracy, precision i recall treba izmestiti iz tih klasa u zaseban modul tako da se kod za evaluaciju klasifikatora ne kopira po klasifikator modulima, te da sutra mogu da se dodaju drugi moduli koji realizuju naprednije tehnike za evaluaciju klasifikatora (npr. nested K-fold cross-validation).

Svi scikit-learn modeli su izmešteni u zaseban fajl a classifier klasa je nadograđena da radi sa bilo kojim scikit-learn modelom. Molim @milsav da pogleda kad stigne da li je to to što je trebalo.

u konstruktoru za GCNEmbedding parametar add_self_loop=False mi se cini suvisnim, u samom konstruktoru je moguce napraviti jednostavnu proveru da li treba dodavati self-loops ili ne, a sa tim parametrom GCNEmbedding tesko ide u embfactory. Alternativa je da se u dataset_pool napravi metod koji za dataset vraca da li treba dodavati self-loops ili ne i onda nakon te izmene izmeniti i postojeci embfactory. Mislim da je prvi pristup jednostavniji i cistiji.

Ovo je ispalo super na kraju, našao sam u kodu DGL-a kako radi provera za izolovane čvorove, tako da sad imamo automatsku detekciju da li treba self loop.

---

Ostalo je nekoliko stvari pa bih molio za pomoć ostale:

Dodati nedostajuće docstringove makar za module, ako nema vremena da dodajemo za sve funkcije

Ne vredi da ja komentarišem kod koji ne razumem, molio bih da svako svoj deo uradi.

Pogledati promenljive sa kratkim imenima (w, x i slično) i zameniti ih gde je potrebno sa razumljivijim imenima

Isto kao iznad.

[milsav]

Pogledao, to je to. Slazem se oko komentarisanja koda, imenovanja promenljivih i dekompozicije, te sitnije stvari u principu mogu i u hodu da se resavaju a ne oduzimaju puno vremena kada svako sredi svoj modul.

[nmilosev]

Zatvaram ovaj issue, samo koristimo Black sad za formatiranje i to je to.