lecromine / face-recogniser

A program that can recognize a person based on pictures of their faces.
0 stars 0 forks source link

Koodikatselmointi #2

Open salsam opened 8 years ago

salsam commented 8 years ago

Koodi ladattu tiistaina 30.8.2016 kello 12:38.

Hei,

aiheesi oli mielestäni todella kiinnostava, joten olen hieman pettynyt, etten onnistunut saamaan ohjelmaa toimimaan. Koodisi oli kauttaaltaan hyvin kommentoitua ja metodit olivat mielestäni riittävän lyhyitä (ja jos jokin metodi olikin pitempi, siihen oli hyvä perustelu). Javadocissa oli hyvin selitetty eri metodien toimintaa. Koodi oli enimmäkseen helppo lukuista, mutta muutamassa metodissa taisi olla vielä debuggausviestejä. Esimerkiksi PGMReader metodissa readFile tuntuivat try-lausekkeen sisällä ensimmäiset noin 20 riviä turhilta, sillä vaikutti, etteivät nämä rivit oikeastaan tee mitään. Veikkaisin tämän olleen osa debuggausta jossain kohtaa ja unohtuneen tähän. Myös muutamissa muissa metodeissa tuntui olevan turhia muuttujia, jotka ovat varmaan unohtuneet projektiin refaktoroinnissa tai debuggauksessa. Nämä olisi hyvä poistaa lopullista palautusta varten.

Ajaessani ohjelman en saanut sitä oikein toimimaan, vaan ohjelma valitti java.io.FileNotFoundException: /home/sami/Downloads/att_faces/s1\ProjectedFaceMatrix.csv (No such file or directory). Olin siis ladannut AT&T-kirjaston tuossa downloads-kansioon, mutta en saanut valittua tätä kansiota, vaan jouduin menemään alikansioon esim. s1 ja valitsemaan siellä olevan kuvan. Tämäköhän ongelman aiheutti? Kokeilin siirtää yhden kansion s1 kuvan yläkansioon att_faces, mutta tämä ilmeisesti ei ainakaan toiminut. Toisaalta, kun tämän jälkeen valitsin kuvan ja sitten recognize, sain ArrayIndexOutOfBoundsExceptionin (10340) PGMreaderin readFilessä, joten siinä on ilmeisesti varattu liian vähän tilaa facevectorille. Toinen vaihtoehto on, ettei satunnaisesti valitsemani Harry Potterin kuva sopinut menetelmään, vaan kuville oli tarkemmat vaatimukset kuin ymmärsin.

Olisin siis kaivannut tarkempia käyttöohjeita tai idiootinvarmempaa ohjelmaa. Suosittelisin myös tallentamaan AT&T-kirjaston resources-kansioon, jos se ei ole liian iso tai muuten mahdoton toteuttaa. Näin voisit suoraan viitata tähän kansioon ja ohjelman käyttö tulisi käyttäjällekin paljon helpommaksi.

Aloin myös miettimään yksikkötestejä, sillä sinulla oli niitä melko vähän. En ole varma kurssin käytännöstä, mutta jos pyrit saamaan korkean arvosanan, luulen, että tarvitset enemmän testejä. Esimerkiksi CSVReaderilla voisi testata myös load metodia jne.

lecromine commented 8 years ago

Kiitokset hyvästä palautteesta. Ehdotuksesi, kannattaa siirtää at&t:n sisältö projektiin mukaan, on kieltämättä ihan järkevä ja helpottaisi ohjelman käyttämistä suuresti. Ohjelma siis tällä hetkellä kyllä osaa tunnistaa kasvoja, mutta on vielä kovin buginen muun moassa kasvojen syöttämisen puolelta.

AT&T-kirjaston ulkopuolelta tuomasi kuva ei toiminut luultavasti siksi, koska kuvien tulee olla saman muotoisia. Tätä varten voisi varmaankin toteuttaa jonkinlaisen croppaussysteemin kuvien lataamisvaiheeseen.