-
Notifications
You must be signed in to change notification settings - Fork 90
Add documented API params setters #1077
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
Conversation
8248538 to
a0a553d
Compare
Codecov ReportAttention: Patch coverage is
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. |
0378d7a to
b20d45b
Compare
gkirgizov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Будет ли пользователю удобно искать, какой параметр к какому блоку относится? Например, мне нужно задать только метрику оптимизации -- как понять, какой with_... написать?
fedot/api/main.py
Outdated
| 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, | ||
| ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Объясни, как эти неиспользуземые аргументы затем попадают дальше в оптимизаторы и тд?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Пока что работаю над этим, чуть поспешили с запросом на ревью. Запрошу повторный ревью, как допилю функционал
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One-hot чаще встречается.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Исправил
fedot/api/main.py
Outdated
| 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'``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не убрал исходное описание до твоих правок чуть ниже.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Исправлено
fedot/api/main.py
Outdated
| Returns: | ||
| the array with prediction values | ||
| The array with prediction values. |
There was a problem hiding this comment.
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, чтобы подчеркнуть, что возвращается "некоторый" массив?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Актуально и для других подобных мест
There was a problem hiding this comment.
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 | ||
| ~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Актуально для других мест тоже.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Почему нельзя написать один раз data, а внутренность по контексту будет понятна?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Можно, конечно. Я просто обновил стиль докстринги, содержание не трогал. Поменяю
Я надеюсь, что из названия методов должно быть понятно. Например, вызываешь Ещё можно сделать блоки параметров пересекающимися, если параметры подходят по контексту в несколько групп. |
a0a553d to
14386bb
Compare
14386bb to
cc90eed
Compare
|
Hello @MorrisNein! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-10-18 06:15:32 UTC |
|
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 |
|
Переделал состав параметров в группах. Насколько удобно получилось -- посмотрите, пожалуйста. Есть альтернативная идея сделать отдельные сеттеры для каждого из параметров API на этапе установки библиотеки. Наподобие того, как это реализовано в matplotlib. Что-то вроде Но эта реализация несколько более муторная, возможно, даже есть смысл посмотреть в сторону программной генерации кода. |
aa693f6 to
7e03668
Compare
Данное решение уже достаточно хорошо. Думаю, пока не возникнут новые неудобства, можно на этом остановиться. Класс |
398c45c to
a171c9f
Compare
eda0813 to
8de2cf4
Compare
6cd91b5 to
2e79a45
Compare
f2ef5cf to
2294403
Compare
2294403 to
70c0c59
Compare
|
Благодаря написанным тестам нашёл пропущенный параметр keep_history. Он был не задокументирован в основном API, и поэтому изначально не попал в FedotBuilder. Так что буду считать покрытие нового функционала тестами вполне достаточным для начала. |
Change list:
FedotBuilderclass used for constructingFedotinstances.Motivation:
Existing documented kwargs are divided in logical blocks, each of them got its separate setter in new
FedotBuilderclass:timeouttask_paramsseedpresetwith_tuninguse_meta_rulesn_jobsparallelization_modelogging_levelshow_progresscache_dirhistory_dirinitial_assumptionnum_of_generationsearly_stopping_iterationsearly_stopping_timeoutpop_sizekeep_n_bestgenetic_schemeuse_pipelines_cacheoptimizeravailable_operationsmax_depthmax_aritymetriccv_foldsmax_pipeline_fit_timecollect_intermediate_metricsafe_modeuse_input_preprocessinguse_preprocessing_cacheUsage examples