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

Metrics fixes, tests, refactoring, typing improvement #1214

Merged
merged 33 commits into from
Dec 6, 2023
Merged

Conversation

MorrisNein
Copy link
Collaborator

@MorrisNein MorrisNein commented Nov 29, 2023

Changes:

  • Fix F1 & precision evaluation
  • Remove flattening of target & predict values during metrics evaluation
  • Unwrap the shape of prediction in pipeline.predict() from (1, N) to (N) for forecasting tasks
  • Add duck typing for MetricCallable
  • Multiple inheritance fixes for the inheritors of Metric
  • Add unit tests for all supported metrics, tasks and data types

@pep8speaks
Copy link

pep8speaks commented Nov 29, 2023

Hello @MorrisNein! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 277:121: E501 line too long (123 > 120 characters)

Line 104:52: F811 redefinition of unused 'data_setup' from line 14

Comment last updated at 2023-11-29 18:17:04 UTC

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (9d9469f) 79.27% compared to head (e1d6f84) 79.42%.
Report is 1 commits behind head on master.

Files Patch % Lines
fedot/core/operations/atomized_model.py 0.00% 29 Missing ⚠️
fedot/core/composer/metrics.py 98.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1214      +/-   ##
==========================================
+ Coverage   79.27%   79.42%   +0.14%     
==========================================
  Files         145      145              
  Lines       10048    10045       -3     
==========================================
+ Hits         7966     7978      +12     
+ Misses       2082     2067      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MorrisNein MorrisNein changed the title Metrics fixes, tests, refactoring Metrics fixes, tests, typing refactoring Nov 29, 2023
@MorrisNein MorrisNein changed the title Metrics fixes, tests, typing refactoring Metrics fixes, tests, refactoring, typing improvement Nov 29, 2023
@kasyanovse kasyanovse self-requested a review November 29, 2023 18:00
fedot/api/builder.py Outdated Show resolved Hide resolved
fedot/api/builder.py Show resolved Hide resolved
fedot/core/composer/metrics.py Show resolved Hide resolved
fedot/core/repository/metrics_repository.py Outdated Show resolved Hide resolved
fedot/core/composer/metrics.py Outdated Show resolved Hide resolved
fedot/core/composer/metrics.py Outdated Show resolved Hide resolved
test/unit/composer/test_metrics.py Outdated Show resolved Hide resolved
test/unit/composer/test_metrics.py Outdated Show resolved Hide resolved
test/unit/composer/test_metrics.py Show resolved Hide resolved
# Time series forecasting
is_ts_forecasting = reference_data.task.task_type == TaskTypesEnum.ts_forecasting
if is_ts_forecasting or is_multi_target_regression:
results, reference_data = cls.flatten_convert(results, reference_data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Почему убрана эта строка?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Потому что она преобразует мульти-таргет предсказания из матрицы в колонку. Вроде как для вычисления метрик нет такой необходимости.

Насчёт преобразования временного ряда из нескольких колонок в одну, мне кажется, тоже очень спорная логика. Но могу ошибаться

Copy link
Collaborator

Choose a reason for hiding this comment

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

Потестировал. Все метрики толерантны к форме данных на входе.

@MorrisNein MorrisNein force-pushed the fix-metrics branch 3 times, most recently from b6c3c5f to beb6bbe Compare December 4, 2023 11:36
@MorrisNein MorrisNein force-pushed the fix-metrics branch 5 times, most recently from 92d32b7 to 17073c1 Compare December 4, 2023 12:41
@MorrisNein
Copy link
Collaborator Author

@kasyanovse, вынес проблему с бинарной классификацией в отдельную issue.

А пока что вынес этот случай в менее строгий тест.

Copy link
Collaborator

@kasyanovse kasyanovse Dec 6, 2023

Choose a reason for hiding this comment

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

  1. Я добавил сид для теста метрик
  2. Убрал простой тест для бинарной классификации
  3. Убрал автоматическую перезапись результатов расчета метрик
  4. Перенес перезапись результатов расчета метрик после assert

У меня небольшая проблема возникла, что в результате теста переписываются все метрики для временных рядов. Пока посмотрю из-за чего, потом аппрувну, если все будет ок.

@MorrisNein MorrisNein merged commit 4ef1c8e into master Dec 6, 2023
5 of 6 checks passed
@MorrisNein MorrisNein deleted the fix-metrics branch December 6, 2023 11:56
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