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

Add desc to not implemented errors #1177

Merged
merged 13 commits into from
Oct 2, 2023

Conversation

valer1435
Copy link
Collaborator

Closes #259

@aim-pep8-bot
Copy link

aim-pep8-bot commented Sep 28, 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-10-02 07:25:40 UTC

@pep8speaks
Copy link

pep8speaks commented Sep 28, 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-10-02 07:25:43 UTC

@valer1435
Copy link
Collaborator Author

работает примерно так
image

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #1177 (20995c4) into master (a2cabc8) will decrease coverage by 0.25%.
The diff coverage is 48.78%.

@@            Coverage Diff             @@
##           master    #1177      +/-   ##
==========================================
- Coverage   79.89%   79.65%   -0.25%     
==========================================
  Files         141      141              
  Lines        9796     9812      +16     
==========================================
- Hits         7827     7816      -11     
- Misses       1969     1996      +27     
Files Coverage Δ
fedot/core/composer/gp_composer/gp_composer.py 88.63% <ø> (+1.40%) ⬆️
...ation_implementations/data_operations/decompose.py 94.89% <ø> (+0.89%) ⬆️
...evaluation/operation_implementations/models/knn.py 96.42% <ø> (+1.60%) ⬆️
fedot/core/pipelines/prediction_intervals/main.py 75.30% <100.00%> (-2.47%) ⬇️
...dot/api/api_utils/assumptions/operations_filter.py 95.45% <75.00%> (+0.71%) ⬆️
fedot/core/composer/composer.py 95.23% <66.66%> (+0.23%) ⬆️
fedot/core/data/multi_modal.py 85.43% <0.00%> (ø)
fedot/core/operations/evaluation/classification.py 96.82% <0.00%> (ø)
fedot/core/operations/evaluation/gpu/common.py 52.08% <75.00%> (+3.19%) ⬆️
...aluation/operation_implementations/models/keras.py 30.09% <0.00%> (ø)
... and 12 more

... and 4 files with indirect coverage changes

def from_operations(self, available_operations: List[str]):
raise NotImplementedError('abstract')
raise NotImplementedError(f'Method {stack()[0][3]} not implemented in {self.__class__}')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше писать что это абстрактный метод и его нельзя вызывать. Во всех наследниках этого класса этот метод будет реализован, никаких проблем с его отсутствием не будет. Абстрактный метод можно вызвать только через super(), так что по идее ошибка может быть только в попытке его вызвать.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Отпишу только здесь, тк по другим неактуально
Добавил классы ошибок

def to_builders(self, initial_node: Optional[PipelineNode] = None,
use_input_preprocessing: bool = True) -> List[PipelineBuilder]:
raise NotImplementedError('abstract')
raise NotImplementedError(f'Method {stack()[0][3]} not implemented in {self.__class__}')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше писать что это абстрактный метод и его нельзя вызывать. Во всех наследниках этого класса этот метод будет реализован, никаких проблем с его отсутствием не будет. Абстрактный метод можно вызвать только через super(), так что по идее ошибка может быть только в попытке его вызвать.

def sample(self) -> str:
""" Samples some operation that satisfies this filter. """
raise NotImplementedError()
raise NotImplementedError(f'Method {stack()[0][3]} not implemented in {self.__class__}')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше писать что это абстрактный метод и его нельзя вызывать. Во всех наследниках этого класса этот метод будет реализован, никаких проблем с его отсутствием не будет. Абстрактный метод можно вызвать только через super(), так что по идее ошибка может быть только в попытке его вызвать.

def ensemble_operation(self) -> str:
""" Suitable ensemble operation used for MultiModalData case. """
raise NotImplementedError()
raise NotImplementedError(f'Method {stack()[0][3]} not implemented in {self.__class__}')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше писать что это абстрактный метод и его нельзя вызывать. Во всех наследниках этого класса этот метод будет реализован, никаких проблем с его отсутствием не будет. Абстрактный метод можно вызвать только через super(), так что по идее ошибка может быть только в попытке его вызвать.

def processing_builders(self) -> List[PipelineBuilder]:
""" Returns alternatives of PipelineBuilders for core processing (without preprocessing). """
raise NotImplementedError()
raise NotImplementedError(f'Method {stack()[0][3]} not implemented in {self.__class__}')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше писать что это абстрактный метод и его нельзя вызывать. Во всех наследниках этого класса этот метод будет реализован, никаких проблем с его отсутствием не будет. Абстрактный метод можно вызвать только через super(), так что по идее ошибка может быть только в попытке его вызвать.


@staticmethod
@abstractmethod
def metric(reference: InputData, predicted: OutputData) -> float:
raise NotImplementedError()
raise NotImplementedError(f'Method {stack()[0][3]} not implemented in {self.__class__}')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше писать что это абстрактный метод и его нельзя вызывать. Во всех наследниках этого класса этот метод будет реализован, никаких проблем с его отсутствием не будет. Абстрактный метод можно вызвать только через super(), так что по идее ошибка может быть только в попытке его вызвать.

@@ -61,7 +62,7 @@ class QualityMetric:
@staticmethod
@abstractmethod
def metric(reference: InputData, predicted: OutputData) -> float:
raise NotImplementedError()
raise NotImplementedError(f'Method {stack()[0][3]} not implemented')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше писать что это абстрактный метод и его нельзя вызывать. Во всех наследниках этого класса этот метод будет реализован, никаких проблем с его отсутствием не будет. Абстрактный метод можно вызвать только через super(), так что по идее ошибка может быть только в попытке его вызвать.

@@ -14,12 +15,12 @@ class DataDetector:
@staticmethod
@abstractmethod
def prepare_multimodal_data(dataframe: pd.DataFrame, columns: List[str]) -> dict:
raise NotImplementedError()
raise NotImplementedError(f'Method {stack()[0][3]} not implemented')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше писать что это абстрактный метод и его нельзя вызывать. Во всех наследниках этого класса этот метод будет реализован, никаких проблем с его отсутствием не будет. Абстрактный метод можно вызвать только через super(), так что по идее ошибка может быть только в попытке его вызвать.


@staticmethod
@abstractmethod
def new_key_name(data_part_key: str) -> str:
raise NotImplementedError()
raise NotImplementedError(f'Method {stack()[0][3]} not implemented')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше писать что это абстрактный метод и его нельзя вызывать. Во всех наследниках этого класса этот метод будет реализован, никаких проблем с его отсутствием не будет. Абстрактный метод можно вызвать только через super(), так что по идее ошибка может быть только в попытке его вызвать.

@@ -38,7 +38,6 @@ def test_prediction_intervals(params):
pred_ints = PredictionIntervals(model=params['model'], method=x, horizon=20, params=pred_ints_params)
pred_ints.fit(params['train_input'])
res = pred_ints.forecast()
pred_ints.plot()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Этот момент я у себя тоже пофиксил))
#1174

def ensemble_operation(self) -> str:
""" Suitable ensemble operation used for MultiModalData case. """
raise NotImplementedError()
raise NotImplementedError(f'Method {stack()[0][3]} is not implemented in {self.__class__}')
Copy link
Collaborator

Choose a reason for hiding this comment

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

stack()[0][3] бы в какую-то служебную функцию вынести.

А конкретно для этого кейса - вообще целиком вся строка "raise NotImplementedError(f'Method {stack()[0][3]} is not implemented in {self.class}')" может быть вынесена.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Добавил классы ошибок. Один в итоге сейчас не нужен. Но может понадобится в будущем.

@valer1435 valer1435 merged commit 88a2264 into master Oct 2, 2023
5 checks passed
@valer1435 valer1435 deleted the add_desc_to_not_implemented_errors branch October 2, 2023 07: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.

Get rid of non-descriptive NotImplementedError's
5 participants