Zadanie #5274

Code review: CMake i klasa Tagger

Added by Adam Radziszewski over 8 years ago. Updated over 8 years ago.

Status:ZamkniętyStart date:27 Mar 2014
Priority:NormalnyDue date:
Assignee:Radosław Warzocha% Done:

0%

Category:-
Target version:python

Description

Przyjrzałem kodowi na branchu rewrite-c i mam pierwsze uwagi.

Struktura katalogów wygląda super:
  • jest główny CMakeLists.txt
  • jest podkatalog libwcrft i widać po obecnej zawartości, że idzie to w dobrym kierunku
  • powyższe podkatalogi mają CMakeLists.txt (nie wnikałem w ich treść)
  • główny CMakeLists.txt załącza póki co tego od liba
CMake jeszcze nie jest funkcjonalny:
  • wywala się na nieistniejącym pliku classify.cpp
    (chyba, że plik u Ciebie jest, a nie ma go na repo? najnowszy commit widoczny na branchu rewrite-c to 10ded)
Za to zmian wymaga interfejs klasy Tagger:
  • Niepraktyczne jest przyjmowanie i zwracanie std::vector<…Ptr>
  • Zamiast tego lepiej zrobić interfejs bliski temu, co jest w pytonie, tj.:
void tag_sentence(SentencePtr sent, bool preserve_ambiguity);
void tag_input(
    const std::string &in_path, // if "" then reads from stdin
    const std::string &out_path, // if "" then writes to stdout
    const std::string &input_format, // e.g. "text", "xces" 
    const std::string &output_format, // e.g. "xces,flat", "ccl" 
    bool preserve_chunks = true, // WIEM, że to było domyślnie false w pytonie, ale to był bład :D
    bool preserve_ambiguity = false
);

void train_and_save(const std::string &in_path, const std::string &input_format);

Podobnie z disambiguate_sentence — niech bierze tylko SentencePtr i niech to będzie protected.

  • Nie jestem też przekonany co do konstruktora. Konstruktor powinien przyjmować możliwie proste typy, by dało się to potem łatwo opakować w Pythona (pewnie też Javę potem).
  • Rozwiązanie z nazwą pliku do configa jako string z kodu pytonowego wydaje mi się dobre
  • Zostawiłbym więc dwa stringi (config_path i data_dir) oraz ten bool verbose = false (chyba, że widzisz jakieś argumenty, by jednak to zmienić, jestem otwarty)

History

#1 Updated by Adam Radziszewski over 8 years ago

Mam sporo uwag do ostatniej wersji (z poniedziałku — 7 kwietnia).
Obecna wersja ma niewiele wspólnego z zaplanowanymi założeniami.

W szczególności podczas ostatniej rozmowy ustaliliśmy, że do klasy Wcrft przenosimy funkcje opisane wyżej, które same tworzą reader i writery tj.:

void tag_input(
    const std::string &in_path, // if "" then reads from stdin
    const std::string &out_path, // if "" then writes to stdout
    const std::string &input_format, // e.g. "text", "xces" 
    const std::string &output_format, // e.g. "xces,flat", "ccl" 
    bool preserve_chunks = true, // WIEM, że to było domyślnie false w pytonie, ale to był bład :D
    bool preserve_ambiguity = false
);

void train_and_save(const std::string &in_path, const std::string &input_format);

Funkcja tagująca string w string jest kontrowersyjna i jeśli w ogóle jest tu dla niej miejsce, to powinna się nazywać tak, by użytkownik wiedział, że należy tego unikać (to jest koszmarnie niewydajne, trzeba cały tekst załadować do jednego wielkiego stringa i potem generuje całe wyjście do bufora i robi z niego stringa). Chodzi mi o funkcję z obecnego kodu:

std::string tag_input(std::string input);

Jeśli już, to powinna się nazywać np. tag_input_from_string i w komentarzu dodać coś w stylu „Warning: this may be very inefficient, especially for longer input. It is strongly recommended to use tag_input instead unless you really know what you are doing.”

Poza tym, nie możemy tutaj zakładać, że na wyjściu jest iob_chan. Format powinien być tak samo parametryzowalny jak w przypadku pozostałych funkcji — const std::string &input_format oraz const std::string &output_format.

Kolejna rzecz to uczenia tagera. Nie da się nauczyć tagera jednym zdaniem / akapitem. Ta funkcja nie ma sensu. Proponuję trzymać się API, które jest w pytonie, znowu input format i pozostałe parametry readera.

#2 Updated by Radosław Warzocha over 8 years ago

  • Status changed from Nowy to Gotowy

Co się zmieniło i dlaczego (powinienem był napisać wcześniej, ale niestety byłem trochę zawalony i zapomniałem):
Tomek powiedział mi, że potrzebuje udostępnienia określonego interfejsu (klasa Wcrft)
wobec tego interfejs, który ustaliliśmy przeniosłem do klasy TaggerIO.

Zgodnie z tym o czym rozmawialiśmy dziś zaraz po raporcie ustalimy ostateczny interfejs i zrobimy refaktoring.
Tymczasowo zamykam ticket.

#3 Updated by Adam Radziszewski over 8 years ago

  • Status changed from Gotowy to Zamknięty

Also available in: Atom PDF