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 documented API params setters #1077

Merged
merged 14 commits into from
Oct 18, 2023
Merged

Add documented API params setters #1077

merged 14 commits into from
Oct 18, 2023

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

Merging #1077 (2294403) into master (a5a9b17) will increase coverage by 0.00%.
Report is 1 commits behind head on master.
The diff coverage is 78.17%.

❗ Current head 2294403 differs from pull request most recent head 70c0c59. Consider uploading reports for the commit 70c0c59 to get more accurate results

@@           Coverage Diff            @@
##           master    #1077    +/-   ##
========================================
  Coverage   79.54%   79.54%            
========================================
  Files         142      145     +3     
  Lines        9805     9995   +190     
========================================
+ Hits         7799     7951   +152     
- Misses       2006     2044    +38     
Files Coverage Δ
fedot/__init__.py 100.00% <100.00%> (ø)
fedot/api/__init__.py 100.00% <100.00%> (ø)
fedot/api/api_utils/presets.py 87.50% <100.00%> (ø)
fedot/api/main.py 63.33% <ø> (ø)
fedot/core/data/data_preprocessing.py 93.50% <100.00%> (+0.26%) ⬆️
fedot/core/data/data_split.py 94.73% <ø> (ø)
...ore/operations/evaluation/evaluation_interfaces.py 86.48% <ø> (-0.13%) ⬇️
...mentations/data_operations/categorical_encoders.py 98.07% <100.00%> (+0.07%) ⬆️
fedot/preprocessing/preprocessing.py 97.00% <100.00%> (+0.01%) ⬆️
fedot/api/builder.py 97.50% <97.50%> (ø)
... and 4 more

... and 3 files with indirect coverage changes

@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.

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

@@ -9,7 +9,7 @@ To tune pipeline hyperparameters you can use GOLEM. There are two ways:
More information about these approaches can be found
`here <https://towardsdatascience.com/hyperparameters-tuning-for-machine-learning-model-ensembles-8051782b538b>`_.

If ``with_tuning`` flag is set to ``True`` when using `FEDOT API`_, simultaneous hyperparameters tuning is applied for composed pipeline and ``metric`` value is used as a metric for tuning.
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

docs/source/advanced/sensitivity_analysis.rst Outdated Show resolved Hide resolved
docs/source/api/sensitivity.rst Outdated Show resolved Hide resolved
@@ -19,7 +19,8 @@ def __init__(self, data: InputData):
"""
Class for handling operations related with assumptions

: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.

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

@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
4 of 5 checks passed
@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.

None yet

5 participants