jayzstep / tiralabra

0 stars 0 forks source link

Vertaisarviointi #2

Open sonjaolkkonen opened 4 months ago

sonjaolkkonen commented 4 months ago

Vertaisarviointi

Projekti ladattu 20.6.2024 klo 3.25

Projektisi on todella hyvällä mallilla ja siihen oli mielenkiintoista tutustua!

Koodin laatu ja selkeys

Koodi on selkeää ja funktiot sekä muuttujat on nimetty järkevästi. Miettisin vielä tiedostorakennetta, nyt kaikki koodi on yhdessä ja samassa tiedostossa. Voisiko ainakin ohjelmalogiikan ja käyttöliittymän eriyttää toisistaan? Myös NN-luokka voisi olla järkevää eriyttää omaksi tiedostoksi.

Huomasin koodin laatuun liittyen, että ilmeisesti pylintiä ei oltu vielä otettu käyttöön? Koodi oli kuitenkin jo nyt selkeää ja laadukasta, mutta pylintin käyttöönotto helpottaisi vielä koodin laadun ylläpitämistä. Testasin pylintin ajamista projektissa, ja sieltä löytyi jonkin verran korjattavaa, lähinnä liian pitkiä rivejä, turhia tyhjiä välilyöntejä muita melko helposti muokattavia "virheitä".

Docstringit olivat selkeitä ja helpottivat koodin ymmärtämistä!

Ohjelman käyttö

UI on simppeli ja helppo käyttää. Tosin nyt Chromella kaikkien numeroiden raahaus ei onnistunut, mutta tämä taisikin olla jo tiedossa. Mikäli raahausta ei saa toimimaan kaikilla selaimilla, olisiko parempi jättää se kokonaan pois ja pitää käytössä ainoastaan numeroiden klikkaus? Klikkaus on omasta mielestäni muutenkin hieman käyttäjäystävällisempi käyttötapa, mutta nämä on varmasti myös makukysymyksiä.

Antaessa ohjelmalle ensimmäisen rivin toisena olevan nollan (neljäs numero), antaa ohjelma tulokseksi 9.

Screenshot from 2024-06-20 03-38-45

Nopealla testauksella ohjelma näyttäisi antavan suurimmaksi osaksi oikean tuloksen, yksittäisiä poikkeuksia lukuunottamatta (edellä olevan esimerkin lisäksi esim. viimeisen sivun toisiksi alimman rivin toinen 3 antaa tulokseksi 2). Osumatarkkuus taisikin olla nyt noin 90%, joten tulokset ovat linjassa tämän kanssa. Voisiko käyttäjälle antaa ilmoituksen, mikäli tulos on väärin?

Mikäli numeron annettua painaa nappia "clear", antaa ohjelma tulokseksi errorin.

Screenshot from 2024-06-20 03-39-01

Terminaalin puolella herjaksi tulee:

File "/../tiralabra/NumNet/numnet/nn.py", line 212, in predict_digit image = image.reshape(784, 0) ValueError: cannot reshape array of size 784 into shape (784,0)

Gradio on itselleni entuudestaan tuntematon, joten valitettavasti en osaa tähän suoraan antaa korjausehdotusta. Mutta tarkista vielä tämä.

Testaus

Testit onnistui ajaa moitteettomasti ja testikattavuus näyttäisi olevan nyt 66%. Toisaalta testikattavuusraportti sisältää myös käyttöliittymän, jonka metodeita ei tarvitse testata. Tähän auttaisi jälleen eriytetty ohjelmalogiikka ja käyttöliittymä, jolloin omassa tiedostossaan oleva käyttöliittymän koodi olisi helppo tiputtaa testikattavuudesta pois.

Olemassa olevat testit näyttävät kattavilta, ja näihin en keksi lisättävää/muokattavaa. Sigmoidin osin mietin, että olisko kannattavaa testata esim. sigmoid_prime-metodi positiivisella ja negatiivisella syötteillä, nollalla sekä suurilla positiivisilla ja negatiivisilla syötteillä? Sigmoid-metodin puolestaan voisi ehkä testata samoilla syötteillä kuin sigmoid_prime-metodi, mutta lisäksi myös positiivisella ja negatiivisella inf-syötteellä.

Kaiken kaikkiaan hyvää työtä! :)

jayzstep commented 4 months ago

Jes mainiota! Hyviä pointteja! Olin pylintillä tsekannut sen verran että ongelmat on aika pieniä, ajattelin että fiksaan ne sitten kun kaikki on valmista :) Joo raahaaminen ei onnistu Chromella ja sitä vaihtoehtoa ei saa pois Gradiosta, mutta olen yrittänyt ohjeistaa että klikatkaa kuvaa. Ehkä laitan sen vielä selkeämmin johonkin, niin käyttäjä ei ihmettele. Tuo Clear-error on ihan itseaiheutettu. Laitoin testejä varten valueError-tsekin, jota se nyt sitten herjaa, jos kuvaa ei ole valittuna. Gradio on mukavan simppeli UI ML-jutuille, mutta se ei taivu oikein mihinkään näpertelyyn. Ehkä laitan jonkun jesaripaikkauksen nyt vain.. :) Testit on seuraavana hiomis/viimeistelylistalla, otan ehdottomasti neuvosta vaarin ja katson jos saisin helposti UI:n eriytettyä, niin saisi coverage-luvut vähän nätimmiksi.