-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mulltimodal pipeline improvement #581
Conversation
Hello @andreygetmanov! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2022-04-07 14:19:52 UTC |
3000903
to
b5e8871
Compare
train_text, test_text) | ||
|
||
pipeline.fit(input_data=fit_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В датасете много классов + он достаточно дисбалансный, поэтому при разделении на train/test не все классы попадали в обе выборки. Поменял датасет, сократил число классов, теперь должно работать.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
поэтому при разделении на train/test не все классы попадали в обе выборки
Хорошо бы это починить, конечно. Но можно отдельным PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хорошо бы это починить, конечно.
В этом примере используется небольшой датасет. Подумал, что поменять датасет и сократить число классов - норм идея для демонстрации примера. Но можно и что-то другое сделать, наверное.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Думаю пока можно действительно оставить упрощенный вариант, а уже последующими PR организовать возможность нормально работать с сильно несбалансированными датасетами
b5e8871
to
a312679
Compare
093fc5c
to
3d89d56
Compare
3d89d56
to
486959d
Compare
486959d
to
fb6f8c5
Compare
Codecov Report
@@ Coverage Diff @@
## master #581 +/- ##
==========================================
- Coverage 86.51% 86.47% -0.05%
==========================================
Files 153 153
Lines 11208 11230 +22
==========================================
+ Hits 9697 9711 +14
- Misses 1511 1519 +8
Continue to review full report at Codecov.
|
0c27d48
to
32353a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ещё стоит написать тест на мультимодальную функциональность, которая уже есть. Вроде таких тестов ещё нет, по крайней мере я не нашёл для случая "картинки + текст + таблицы"
8c1b416
to
6cd90a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В целом все ок, осталось только с random_state = 42 ещё разобраться. Может @nicl-nno подскажет стоит ли вводить тут определение random_state через randint, чтобы разбиение все таки отличалось от запуска к запуску. Или это для чего-то конкретного так задумывалось?
Ну и unit тест ещё нужен - стоит не забыть |
https://github.com/nccr-itmo/FEDOT/blob/master/test/unit/pipelines/test_multi_modal.py Этот тест генерирует и запускает пайплайн для случая "картинки + текст + таблицы". Нужно как-то дополнить его функционал? Не совсем понимаю, что можно в него добавить |
04da0ad
to
3337a68
Compare
Да вроде в примере это не обязательно. |
Для примера - определенно необязательно. Но мой вопрос про то, стоит ли оставлять такой хардкод во внутренних функциях. То есть если random seed не меняется в |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Сделай пожалуйста ребейз, и думаю, можно мерджить
Ок, это действительно можно убрать. Хотя оно и не в этом PR-е появилось. |
Создать issue по этому поводу? |
- fixed the optimizer error in multimodal pipeline - fixed the bug #564 'Example multi_modal_pipeline_genres failed' - deleted the example of rating prediction - optimized the process of NLP libraries import - changed the data for multimodal example - upgraded stemmer from Porter to Snowball - fixed bug of merging multimodal data - fixed bug of multimodal data shuffling while loading - CNN now works on multioutput task - Fixed the bug with incorrect type and shape of multioutput predictions
- removed warning during scaling image data - minor changes for readability - test_multi_modal.py is changed accordingly to new structure of multi_modal_pipeline.py
- now there is no useless try of download of stopwords and other nltk packages if they are already downloaded - keras.Input changed to recommended keras.layers.InputLayer - test_multi_modal.py is moved to multimodal folder
Можешь просто убрать. |
3337a68
to
9716b28
Compare
Ладно, давай уже не в этом PR, предлагаю отдельно снести |
Fixes in composer's work, minor changes and optimization of multimodal tools