Skip to content

Conversation

@MorrisNein
Copy link
Collaborator

@MorrisNein MorrisNein commented Mar 31, 2023

Change list:

  • Create FedotBuilder class used for constructing Fedot instances.
  • Add usage examples
  • Add tests
  • Write documentation

Motivation:

  • make API parameters easier to document for us (actual documented function arguments with type hints instead of a plain docstring);
  • make API parameters easier to understand and set for outer users (code autocompletion, type hints).

Existing documented kwargs are divided in logical blocks, each of them got its separate setter in new FedotBuilder class:

  • setup_composition
    • timeout
    • task_params
    • seed
    • preset
    • with_tuning
    • use_meta_rules
  • setup_parallelization
    • n_jobs
    • parallelization_mode
  • setup_output
    • logging_level
    • show_progress
    • cache_dir
    • history_dir
  • setup_evolution
    • initial_assumption
    • num_of_generations
    • early_stopping_iterations
    • early_stopping_timeout
    • pop_size
    • keep_n_best
    • genetic_scheme
    • use_pipelines_cache
    • optimizer
  • setup_pipeline_structure
    • available_operations
    • max_depth
    • max_arity
  • setup_pipeline_evaluation
    • metric
    • cv_folds
    • max_pipeline_fit_time
    • collect_intermediate_metric
  • setup_data_processing
    • safe_mode
    • use_input_preprocessing
    • use_preprocessing_cache

Usage examples

@MorrisNein MorrisNein force-pushed the api_params_setters branch 2 times, most recently from 8248538 to a0a553d Compare March 31, 2023 19:03
@MorrisNein MorrisNein changed the base branch from master to api_doc March 31, 2023 19:03
@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Attention: Patch coverage is 97.67442% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.55%. Comparing base (a5a9b17) to head (70c0c59).
Report is 63 commits behind head on master.

Files Patch % Lines
fedot/api/builder.py 97.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1077      +/-   ##
==========================================
+ Coverage   79.54%   79.55%   +0.01%     
==========================================
  Files         142      145       +3     
  Lines        9805     9995     +190     
==========================================
+ Hits         7799     7952     +153     
- Misses       2006     2043      +37     

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

@nicl-nno nicl-nno requested review from IIaKyJIuH and gkirgizov March 31, 2023 19:58
@aimclub aimclub locked and limited conversation to collaborators Apr 1, 2023
@aimclub aimclub unlocked this conversation Apr 1, 2023
@MorrisNein MorrisNein force-pushed the api_doc branch 3 times, most recently from 0378d7a to b20d45b Compare April 3, 2023 16:16
Base automatically changed from api_doc to master April 3, 2023 16:24
Copy link
Collaborator

@gkirgizov gkirgizov left a comment

Choose a reason for hiding this comment

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

Будет ли пользователю удобно искать, какой параметр к какому блоку относится? Например, мне нужно задать только метрику оптимизации -- как понять, какой with_... написать?

Comment on lines 120 to 127
def with_composer_params(
self, *ignore,
show_progress: Optional[bool] = None,
preset: Optional[str] = None,
with_tuning: Optional[bool] = None,
use_meta_rules: Optional[bool] = None,
cache_dir: Optional[str] = None,
history_dir: Optional[str] = None,
):
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 Author

Choose a reason for hiding this comment

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

Теперь попадают через основной апи)
Сеттеры добавил в отдельную сущность

safe_mode: if set ``True`` it will cut large datasets to prevent memory overflow and use label encoder
instead of oneHot encoder if summary cardinality of categorical features is high.
instead of OneHot encoder if summary cardinality of categorical features is high.
Copy link
Collaborator

Choose a reason for hiding this comment

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

One-hot чаще встречается.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Исправил

n_jobs: num of ``n_jobs`` for parallelization (set to ``-1`` to use all cpu's). Defaults to ``-1``.
parallelization_mode (str): type of evaluation for groups of individuals (``'populational'`` or
``'sequential'``). Default value is ``'populational'``.
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.

Исправлено

Returns:
the array with prediction values
The array with prediction values.
Copy link
Collaborator

Choose a reason for hiding this comment

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

По-моему, не очень понятно, что это за конкретное The array, Может показаться, что изменяется входной features, не лучше ли An array, чтобы подчеркнуть, что возвращается "некоторый" массив?

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.

Везде, где увидел, поправил

If ``with_tuning`` flag is set to ``True`` when using :doc:`FEDOT API </api/api>`, simultaneous hyperparameters tuning is applied for composed pipeline and ``metric`` value is used as a metric for tuning.

Simple example
~~~~~~~~~~~~~~
Copy link
Collaborator

Choose a reason for hiding this comment

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

Я понимаю, что это рекомендация, но мб для единообразия попробовать следовать этим принципам?

image

Если да, то под "=" должен быть "-"

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.

Это больше не входит в данный PR

:param data: data for pipelines
Args:
data: data for pipelines.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Почему нельзя написать один раз data, а внутренность по контексту будет понятна?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Можно, конечно. Я просто обновил стиль докстринги, содержание не трогал. Поменяю

@MorrisNein
Copy link
Collaborator Author

MorrisNein commented Apr 12, 2023

Будет ли пользователю удобно искать, какой параметр к какому блоку относится? Например, мне нужно задать только метрику оптимизации -- как понять, какой with_... написать?

Я надеюсь, что из названия методов должно быть понятно. Например, вызываешь with_... и смотришь контекстные подсказки. Метрика относится к оценке пайплайнов -> наверняка нужно выбрать with_pipeline_evaluation_params.

Ещё можно сделать блоки параметров пересекающимися, если параметры подходят по контексту в несколько групп.

@MorrisNein MorrisNein marked this pull request as draft June 21, 2023 12:33
@pep8speaks
Copy link

pep8speaks commented Sep 29, 2023

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

Line 3:1: F403 'from fedot import ' used; unable to detect undefined names
Line 3:1: F401 'fedot.
' imported but unused
Line 4:1: F401 'fedot.api.Fedot' imported but unused
Line 4:1: F401 'fedot.api.FedotBuilder' imported but unused
Line 5:1: F401 'fedot.version.version' imported but unused

Line 1:1: F401 'fedot.api.main.Fedot' imported but unused
Line 2:1: F401 'fedot.api.builder.FedotBuilder' imported but unused

Comment last updated at 2023-10-18 06:15:32 UTC

@aim-pep8-bot
Copy link

aim-pep8-bot commented Sep 29, 2023

Hello @MorrisNein! 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-09-29 15:36:52 UTC

@MorrisNein MorrisNein marked this pull request as ready for review September 29, 2023 15:16
@MorrisNein
Copy link
Collaborator Author

Переделал состав параметров в группах. Насколько удобно получилось -- посмотрите, пожалуйста.

Есть альтернативная идея сделать отдельные сеттеры для каждого из параметров API на этапе установки библиотеки. Наподобие того, как это реализовано в matplotlib.

Что-то вроде FedotBuilder.pop_size(pop_size).

Но эта реализация несколько более муторная, возможно, даже есть смысл посмотреть в сторону программной генерации кода.

@gkirgizov
Copy link
Collaborator

Что-то вроде FedotBuilder.pop_size(pop_size).

Но эта реализация несколько более муторная, возможно, даже есть смысл посмотреть в сторону программной генерации кода.

Данное решение уже достаточно хорошо. Думаю, пока не возникнут новые неудобства, можно на этом остановиться. Класс

@MorrisNein MorrisNein force-pushed the api_params_setters branch 2 times, most recently from 398c45c to a171c9f Compare October 17, 2023 14:29
@MorrisNein MorrisNein force-pushed the api_params_setters branch 2 times, most recently from eda0813 to 8de2cf4 Compare October 17, 2023 14:36
@MorrisNein MorrisNein force-pushed the api_params_setters branch 3 times, most recently from 6cd91b5 to 2e79a45 Compare October 17, 2023 21:06
@MorrisNein MorrisNein merged commit 9cd08a0 into master Oct 18, 2023
@MorrisNein
Copy link
Collaborator Author

Благодаря написанным тестам нашёл пропущенный параметр keep_history. Он был не задокументирован в основном API, и поэтому изначально не попал в FedotBuilder.

Так что буду считать покрытие нового функционала тестами вполне достаточным для начала.

@MorrisNein MorrisNein deleted the api_params_setters branch October 18, 2023 06:18
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.

6 participants