Skip to content
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

Fix of TextDataMerger #1052

Merged
merged 2 commits into from
Mar 6, 2023
Merged

Fix of TextDataMerger #1052

merged 2 commits into from
Mar 6, 2023

Conversation

andreygetmanov
Copy link
Collaborator

  • now text columns from multiple sources can be merged to 1 column (so early fusion strategy is available)
  • test_data_merge_texts is changed

@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Merging #1052 (759f8a1) into master (3a9f98a) will increase coverage by 0.18%.
The diff coverage is 100.00%.

❗ Current head 759f8a1 differs from pull request most recent head fa9db71. Consider uploading reports for the commit fa9db71 to get more accurate results

@@            Coverage Diff             @@
##           master    #1052      +/-   ##
==========================================
+ Coverage   88.13%   88.32%   +0.18%     
==========================================
  Files         130      133       +3     
  Lines        9305     9523     +218     
==========================================
+ Hits         8201     8411     +210     
- Misses       1104     1112       +8     
Impacted Files Coverage Δ
fedot/core/data/merge/data_merger.py 98.00% <100.00%> (+0.12%) ⬆️
fedot/core/caching/preprocessing_cache_db.py 87.17% <0.00%> (-5.13%) ⬇️
fedot/core/caching/preprocessing_cache.py 82.60% <0.00%> (-4.35%) ⬇️
...on_implementations/models/discriminant_analysis.py 88.88% <0.00%> (-3.71%) ⬇️
fedot/api/main.py 81.10% <0.00%> (-0.09%) ⬇️
...entations/models/ts_implementations/statsmodels.py 95.34% <0.00%> (-0.06%) ⬇️
...implementations/models/ts_implementations/naive.py 96.03% <0.00%> (-0.04%) ⬇️
fedot/core/pipelines/node.py 95.48% <0.00%> (ø)
fedot/core/operations/atomized_model.py 67.21% <0.00%> (ø)
fedot/core/pipelines/tuning/search_space.py 100.00% <0.00%> (ø)
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Comment on lines +170 to +171
if any(len(pred.shape) > 2 for pred in predicts):
raise ValueError('Merge of arrays with more than 2 dimensions is not supported')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тут может придти predict с формами [(n, 1), (m, 1)]?

Copy link
Collaborator Author

@andreygetmanov andreygetmanov Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тут может придти predict с формами [(n, 1), (m, 1)]?

Пока мёржер работает только на сшивание текста и текста. Он сшивает столбцы, поэтому если придёт predict, который ты описал, тут два варианта:

  1. Либо мёржер пытается сшить текстовый столбец и таблицу (я пока думаю, насколько есть смысл это делать, пока что польза неочевидна, поэтому и не добавил в PR)
  2. Либо он каким-то образом пытается сшить неодномерный текст. Тогда всё сломается во многих местах, Федот работает с текстовыми признаками как столбцами (n, 1) или (n, ).

Резюмируя: вряд ли, тогда бы всё сломалось. А если и придёт, то оптимизатор просто пропустит этот вариант. Я пока глубоко в историю оптимизации не копал, но во время запуска мультимодальных примеров все популяции в поколении обучались без ошибок

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сюда гарантированно придет 2+текста, без таблиц?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сюда гарантированно придет 2+текста, без таблиц?

Случай с таблицами учитывается (см. сообщения выше). TLDR: сшивать текст с таблицей не нужно, при такой попытке - выбросит Error и пойдёт дальше

@gkirgizov gkirgizov self-requested a review February 23, 2023 06:06
@valer1435
Copy link
Collaborator

Это фикс только для одной из проблем-сливания текстов. Но существует еще одна: иногда в пайплайне полностью отсутствуют data_source узлы. Надо это фиксить до обучения (можно исправить это правило)

Copy link
Collaborator

@valer1435 valer1435 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Только исправить второй баг и ок

@valer1435
Copy link
Collaborator

Не вижу проблемы исправить его прямо тут - не должно занять много времени

@andreygetmanov
Copy link
Collaborator Author

Не вижу проблемы исправить его прямо тут - не должно занять много времени

Изучил работу пайплайн чекеров. Дело в том, что в рамках конкретного пайплайна (если не привязаны данные, а привязка происходит уже после проверок) однозначно определить, является ли пайплайн мультимодальным, невозможно. Чек has_correct_data_sources применяется и к обычным пайплайнам, и к мультимодальным, поэтому если я добавлю условие на наличие хотя бы одной датасурс ноды (что актуально для мультимодалки и неактуально для унимодалки), всё поломается
Я вижу смысл в расширении verification_rules. Думаю добавить multimodal_rules по аналогии с ts_rules. Мне кажется, могут появиться другие правила, которые тоже нужно будет добавлять для мультимодальных данных. Да и такой подход позволит протаскивать внешние параметры/данные, не трогая базовую реализацию
Но это не быстрофикс. Мне кажется, есть смысл реализовать его в моём следующем ПР
@nicl-nno @gkirgizov что скажете?

@nicl-nno
Copy link
Collaborator

nicl-nno commented Mar 6, 2023

Если в этом всё остальное уже готово - то можно и в следующем.

@andreygetmanov
Copy link
Collaborator Author

Если в этом всё остальное уже готово - то можно и в следующем.

Ок, завёл issue

@andreygetmanov andreygetmanov merged commit 35281f6 into master Mar 6, 2023
@andreygetmanov andreygetmanov deleted the text_data_merger_fix branch March 6, 2023 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants