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

Update default params based on tuning search space #137

Merged
merged 7 commits into from
May 17, 2024

Conversation

PvtKaefsky
Copy link
Collaborator

Checked all parameters with those taken into account in the selection of parameters during tuning (tuning/search_space.py).

For new continuous values took average values from those specified in the tuning search space selection.

For the rest I checked the documentation somewhere, somewhere I set them by analogy with similar classes.

@technocreep technocreep self-assigned this May 14, 2024
@technocreep technocreep added bug Something isn't working good first issue Good for newcomers labels May 14, 2024
Copy link
Collaborator

@technocreep technocreep left a comment

Choose a reason for hiding this comment

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

отметил что успел из явных недочётов. Дальше предлагаю самостоятельно проверить себя

"threshold": 20000
},
"topological_extractor": {
"n_jobs": 2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

n_jobs точно нет, да это было бы странно оптимизировать, посмотри снова на search_space и класс

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.

n_jobs тут был изначально, убрать?

И вообще тут ситуация в том, что topological_extractor указан два раза в search space: один раз в industrial_search_space, а другой раз в parameters_per_operation. И получается что в данном случае учитываются параметры обоих. Как тут лучше сделать, оставить только два параметра для industrial_search_space?

Copy link
Collaborator

Choose a reason for hiding this comment

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

n_jobs убрать

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

А что по поводу того, что в search_space он два раза учитывается?

Я так понимаю, лучше убрать его из parameters_per_operation и убрать те параметры, которых нету в атрибутах класса?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

И в целом для проверки topological_extractor не получилось запустить рекомендованный в классе пайплайн (eigen_basis + topological_extractor + rf) ни с какими параметрами.

Copy link
Collaborator

Choose a reason for hiding this comment

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

parameters_per_operation лучше не трогать

Copy link
Collaborator

Choose a reason for hiding this comment

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

пока просто убрать n_jobs

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 Author

Choose a reason for hiding this comment

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

Тюнер также работает, но на фите падает

},
"quantile_extractor": {
"window_mode": false,
"var_threshold": 0.01,
Copy link
Collaborator

Choose a reason for hiding this comment

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

var_threshold захардкожен и не оптимизируется

Copy link
Collaborator

Choose a reason for hiding this comment

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

равно как и window_mode. Такой параметр мы использовали довольно давно, сейчас можно указывать длину окна 0, это видно в search_space

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

убрал window_mode и var_threshold, длину окна поставил 0 (до этого стояло среднее области поиска)

Copy link
Collaborator

Choose a reason for hiding this comment

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

если посмотреть по коду, то var_threshold вообще не используется нигде, поэтому его можно убрать отовсюду, я полагаю. По крайней мере я не нашёл упоминаний, проверь ещё разок, я могу ошибаться

Copy link
Collaborator

Choose a reason for hiding this comment

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

длину окна поставил 0

если так, то из search_space надо это значение убрать – искать оптимум, начиная от 1
объясню, почему: дефолтные параметры используются для самого первого фита пайплайна и предикта. Оптимизатор хранит результаты предикта как бейзлайн. В ходе оптимизации выбираются другие комбинации параметров, благодаря чему получаются иные результаты, которые сравниваются с тем самым бейзлайном и, если они хуже, по итогу на руках у нас остаётся модель с дефолтными параметрами (да, такое бывает)

страйд при окне=0 влияния не оказывает, это и логично. Страйд - это шаг, с которым окно перемещается вдоль временного ряда. Если окно=0, то мы берём ряд целиком – некуда шагать

Copy link
Collaborator

Choose a reason for hiding this comment

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

еще важное уточнение для понимания: window_size тут – процент от длины окна

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Поставил в search_space поиск длины окна от 5 с шагом в 5 по аналогии с другими экстракторами

Comment on lines 61 to 65
"patch_tst_model": {
"epochs": 100,
"batch_size": 32,
"activation": "relu"
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

тут на мой взгляд лучше было бы взять атрибуты из класса и апдейтнуть search_space+defaults:

self.learning_rate = params.get('learning_rate', 0.001)
self.use_amp = params.get('use_amp', False)
self.horizon = params.get('forecast_length', None)
self.patch_len = params.get('patch_len', None)
self.output_attention = params.get('output_attention', False)

но, возможно, у @v1docq другое мнение. Надо посоветоваться

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Тут я пока что добавил в дефолт, но проверить пока не получается, тоже на фите падает при любых параметрах

"omniscale_model": {
"epochs": 100,
"batch_size": 32,
"activation": "softmax"
Copy link
Collaborator

Choose a reason for hiding this comment

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

тут "softmax", а в searc_space 'Softmax'. Надо бы проверить, не влияет ли это на функциональность

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Проверил эти варианты, разницы не заметил

Comment on lines 71 to 74
"inception_model": {
"epochs": 100,
"batch_size": 32,
"activation": "softmax"
Copy link
Collaborator

Choose a reason for hiding this comment

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

тут вроде не хватает num_classes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Добавил

@pep8speaks
Copy link

pep8speaks commented May 14, 2024

Thanks for update, @PvtKaefsky!

Line 8:1: E265 block comment should start with '# '

Comment last updated at 2024-05-17 11:36:21 UTC

@technocreep technocreep merged commit 74b885a into main May 17, 2024
5 checks passed
@technocreep technocreep deleted the fix_default_params branch May 17, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants