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 pos_label #1131

Merged
merged 10 commits into from
Aug 14, 2023
Merged

fix pos_label #1131

merged 10 commits into from
Aug 14, 2023

Conversation

valer1435
Copy link
Collaborator

@valer1435 valer1435 commented Aug 2, 2023

  1. Error with pos_label when passing binary string target was fixed
  2. Error with resample was fixed (add extra rule)
  3. Fitness calculating errors now raise exceptions if it's test
  4. Unexpected behaviour with inplace operations with data in node.fit and node.predict was fixed

@aim-pep8-bot
Copy link

aim-pep8-bot commented Aug 7, 2023

Hello @valer1435! 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 2023-08-14 09:25:14 UTC

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #1131 (b7af0ed) into master (89ff552) will decrease coverage by 0.05%.
The diff coverage is 68.00%.

@@            Coverage Diff             @@
##           master    #1131      +/-   ##
==========================================
- Coverage   78.67%   78.62%   -0.05%     
==========================================
  Files         131      131              
  Lines        9362     9401      +39     
==========================================
+ Hits         7366     7392      +26     
- Misses       1996     2009      +13     
Files Changed Coverage Δ
fedot/core/operations/operation.py 87.64% <ø> (-0.14%) ⬇️
fedot/core/pipelines/verification.py 100.00% <ø> (ø)
...ot/core/composer/gp_composer/specific_operators.py 80.30% <14.28%> (-7.84%) ⬇️
fedot/core/pipelines/pipeline.py 93.10% <50.00%> (-2.14%) ⬇️
fedot/core/pipelines/verification_rules.py 93.68% <78.57%> (-1.24%) ⬇️
fedot/core/pipelines/random_pipeline_factory.py 85.96% <83.33%> (+0.77%) ⬆️
fedot/core/composer/metrics.py 92.75% <84.61%> (-0.14%) ⬇️
fedot/api/api_utils/api_params_repository.py 96.72% <100.00%> (+0.05%) ⬆️

... and 2 files with indirect coverage changes

fedot/core/composer/metrics.py Show resolved Hide resolved
@@ -80,6 +81,7 @@ def fit(self, params: Optional[Union[OperationParameters, dict]], data: InputDat
Returns:
tuple: trained operation and prediction on train data
"""
data = deepcopy(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.

В fit не уверен нужно ли это сейчас, добавил для избежания проблем в будущем.

А в методе predict происходит неочевидное изменение полей у объекта входных данных
image

Copy link
Collaborator Author

@valer1435 valer1435 Aug 9, 2023

Choose a reason for hiding this comment

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

Из-за этого у тебя данные (исходные), прогнанные не через pipeline.predict, а через root_node.predict получают метку о том, что они предобработанны

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.

А это точно нужно именно в operation? Это действительно очень дорого делать deepcopy всех данных для каждого узла в пайплайне.

Кстати не очень понял почему тут вообще ставится флаг obligatorily_preprocessed. Вроде код выше за это не отвечает.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@valer1435 Думаю, что пока не критично. В целом, это проблема для PR по оптимизации используемой памяти

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

А это точно нужно именно в operation? Это действительно очень дорого делать deepcopy всех данных для каждого узла в пайплайне.

Кстати не очень понял почему тут вообще ставится флаг obligatorily_preprocessed. Вроде код выше за это не отвечает.

Да, это дорого, поэтому хотелось бы найти обходной путь. obligatory_preprocessed ставится в predict. Подумаю, может это и не нужно

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Удалил deepcopy и убрал присвоение флага препроцессинга

@valer1435 valer1435 merged commit 04bceac into master Aug 14, 2023
5 checks passed
@valer1435 valer1435 deleted the hotfix-binary-metric branch August 14, 2023 09:36
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