TuuPu / ModifiedHausdorffDistance

1 stars 0 forks source link

Vertaisarviointi 2 #1

Open hhautajarvi opened 2 years ago

hhautajarvi commented 2 years ago

Projektin viimeisin versio ladattu 29.4. noin klo 19:00

Ohjelma vaikuttaa jo hyvin valmiilta ja hiotulta. Koodi on hyvin selkeää ja todella hyvin kommentoitua, vaikea löytää mitään erityistä parannettavaa. Ehkä jonkinlaista luokkajakoa olisi voinut tehdä selkeyden takia, toki ei ehkä tämän suuruusluokan työssä ole välttämätöntä. Samoin dokumentaatio on varsinkin hyvin perusteellista ja antaa hyvän ja ymmärrettävän kuvan ohjelmasta.

Performance_test -osiossa olisi ehkä muutaman käytännössä saman funktion (calculate_distances -funktiot, sekä erikseen time_with_pythons_sort, time_with_heap_search, time_with_using_heapify) voinut yhdistää funktiolla jolle olisi antanut syötteenä käytettävät pituusfunktiot. Tämä karsisi hieman turhaa toistoa.

Testit vaikuttavat fiksuilta ja kattavilta. Ehkä niihin voisi laittaa lyhyet kommentit mitä testataan, sen verran monipuolisia ettei niitä helposti täysin ymmärrä. Pytestin ajamalla tulee ainakin minulla paljon deprecation-warningeja, liekö kannattavaa korjailla?

UI taitaa olla vielä työn alla ja olet ehkä sitä parantamassa nykyisestä, joten osa ehdotuksista saattaa olla jo mielessä. Tällä hetkellä se ei toki ole asiaa työtä hyvin tarkasti tuntemattomalle kovinkaan intuitiivinen. Mitä eroa kahdella eri k-values graafilla esimerkiksi on tai mitä k-arvot tarkoittavat? Ei ole selvää mitkä graafit ovat staattisia tuloksia, ja mitkä vaihtuvat tällä hetkellä näytöllä olevan näytteen perusteella. (Toki vastaukset löytyvät hyvästä dokumentaatiosta, mutta olisi kätevää jos asiat olisi vaikka lyhyesti mainittu käyttöliittymässä) Sort times-graafi ei ainakaan näytä muuttuvan, mutta muuttaa kuitenkin värejä. Vaihtuvissa graafeissa on kyllä ihan mukava että värit vaihtuvat huomaamisen kannalta, toisaalta taas selventeen värit eivät muutu samalla, joten niitä on vaikeampi hahmottaa nopeasti. Samoin näytteiden selaus omaan tahtiin olisi ehkä mukavampi kuin ajastettu vaihtuminen. Ohjelmasta olisi myös mukava olla muukin poistumistie kesken suorituksen kuin prosessin tappaminen.

Hienoa työtä kaikkinensa, vaikuttaa varsin kunnianhimoiselta aiheelta joka on toteutettu hyvin ja varsin ajoissa jo lähes valmiina! Mahdolliseen jatkokehitykseen olisi hauska nähdä esimerkiksi toiminto jossa voisi ladata omia kuviaan tunnistettavaksi!

TuuPu commented 2 years ago

Moikka!

Kiitos tosi hyvästä palautteesta! Luokkajakoa olen itsekin pohtinut. Etenkin UI:n osalta, mutta toistaiseksi päädyin jakamaan projektin vain omiin moduuleihin ja pidin app.py moduulin UI:n pestissä. Myös mainitsemasi performance_test moduulin funktioiden toisteisuus on tarkoitus korjata, nyt kun siirsin sinne nuo funktiot jotka ajavat suorituskykytestit kokonaisuudessaan. Eli hyviä huomioita!

Hyvä pointti testien kommentoimisesta, pyrin tekemään sinnekin kommentit, niin eivät ole niin abstrakteja ulkopuoliselle lukijalle. En tullut ajatelleeksi, mutta kyllähän se dokumentointi pätee testeihinkin! Ja pitänee myös tarkistella nuo varoituksetkin. Olin vähän pihalla niiden kanssa, mutta paneudutaan niihin viimeistelyviikolla!

UI tosiaan työn alla. Mutta se ei välttämättä hirveästi kokonaisuudessaan muutu. Korjaukset jotka mainitsit, tulevat kyllä valmiiseen pakettiin. Eli graafien selitykset, sekä värikorjaus. Jostain syystä matplotlib haluaa joka iteraatiolla vaihtaa graafien värejä (vaikka legendit pysyvät saman värisinä). Mutta tarkoituksenani on näyttää graafit, sekä looppaava vaihtuva numero ja sen äänestys arvot. Voisin myös kokeilla tuota näytteiden vaihtamista omaan tahtiin ja projektin sulkemista silloin, kun haluaa.

Ihan huippu idea myös jatkokehitykseen! Pitänee kokeilla tuota. Kiitos vielä kommenteista. Olivat todella hyödyllisiä ja sain lisää ideoita viimeiselle viikolla!