Zadanie #5274
Code review: CMake i klasa Tagger
Status: | Zamknięty | Start date: | 27 Mar 2014 | |
---|---|---|---|---|
Priority: | Normalny | Due 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
- 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)
- 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 9 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 9 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 9 years ago
- Status changed from Gotowy to Zamknięty