Add topofeatures #1241
Add topofeatures #1241
Conversation
|
Hello @valer1435! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-12-28 11:36:56 UTC |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1241 +/- ##
==========================================
+ Coverage 79.97% 80.05% +0.07%
==========================================
Files 145 149 +4
Lines 9945 10278 +333
==========================================
+ Hits 7954 8228 +274
- Misses 1991 2050 +59 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
До конца не досмотрел. Давай сначала найденные моменты уточним.
Вообще, кажется, что местами итерирование по спискам можно заменить на работу с np.ndarray, что значительно ускорит топологические признаки.
И смущает полное отсутствие тестов для всех новых объектов. Мне кажется, что нужно покрыть все объекты и основные методы.
| @@ -44,7 +46,8 @@ class FedotPreprocessingStrategy(EvaluationStrategy): | |||
| 'poly_features': PolyFeaturesImplementation, | |||
| 'one_hot_encoding': OneHotEncodingImplementation, | |||
| 'label_encoding': LabelEncodingImplementation, | |||
| 'fast_ica': FastICAImplementation | |||
| 'fast_ica': FastICAImplementation, | |||
| 'topological_features': TopologicalFeaturesImplementation | |||
There was a problem hiding this comment.
Почему в препроцессинге? Что на выходе модели?
There was a problem hiding this comment.
Вход табличка (лаг таблица), на выходе таблица (в теории можно запустить и для табличных данных, но думаю пока нецелесообразно)
| from scipy.linalg import hankel | ||
|
|
||
|
|
||
| class HankelMatrix: |
There was a problem hiding this comment.
А в чем глобальная разница между этим классом и LaggedImplementation?
There was a problem hiding this comment.
На самом деле ни в чем) Не было времени реализовать с момощью нашего лаг преобразования
|
|
||
| @staticmethod | ||
| def __create_epsilon_range(epsilon): | ||
| return np.array([y * float(1 / epsilon) for y in range(epsilon)]) |
There was a problem hiding this comment.
Зачем float? Почему не np.arange(epsilon) / epsilon?
| if self.__window_length is None: | ||
| self.__window_length = dimension_embed |
There was a problem hiding this comment.
Свойства с двумя подчеркиваниями нельзя посмотреть извне класса, что при отладке бывает очень неудобно.
| diagrams = [np.array([dg for dg in diag if np.isfinite(dg).all()]) for diag in diagrams] | ||
| diagrams = diagrams / max( | ||
| [np.array([dg for dg in diag if np.isfinite(dg).all()]).max() for diag in diagrams if diag.shape[0] > 0]) |
There was a problem hiding this comment.
Можно переписать быстрее, без повторных вычислений одного и того же.
There was a problem hiding this comment.
Это скорее всего что-то старое и неиспользуемое) Удалил
| feature_list.append(x_features) | ||
| for dim in range(len(x_features)): | ||
| column_list.append('{}_{}'.format(feature_name, dim)) | ||
| except Exception: |
There was a problem hiding this comment.
Из-за чего код может упасть?
| x_features = feature_model.fit_transform(x_pers_diag) | ||
| feature_list.append(x_features) | ||
| for dim in range(len(x_features)): | ||
| column_list.append('{}_{}'.format(feature_name, dim)) |
There was a problem hiding this comment.
Если не ошибаюсь, то f-строки быстрее, чем format.
| for dim in range(len(x_features)): | ||
| column_list.append('{}_{}'.format(feature_name, dim)) | ||
| continue | ||
| x_transformed = pd.DataFrame(data=np.hstack(feature_list)).T |
There was a problem hiding this comment.
В FEDOT не используются pd.DataFrame, ИМХО безопаснее везде использовать np.ndarray, чтобы случайно не словить где-нибудь ошибку.
| for hole in persistence_diagram: | ||
| index = int(hole[2]) | ||
| lifetime = hole[1] - hole[0] | ||
| if np.equal(lifetime, self.ratio_ * max_lifetimes[index]): |
There was a problem hiding this comment.
Почему через np.equal?
| super(PersistenceEntropyFeature).__init__() | ||
|
|
||
| def extract_feature_(self, persistence_diagram): | ||
| persistence_entropy = PersistenceEntropy(n_jobs=-1) |
There was a problem hiding this comment.
По умолчанию все ноды в FEDOT работают в один поток, т.к. распараллеливание осуществляется на уровне пайплайнов.
There was a problem hiding this comment.
Да, мне тоже неприятно, но иначе это работает неприлично долго. Я пока не заметил серьезных проблем из-за этого. Главная проблема у нас из-за распараллеливания была с памятью - тут ее много не надо. Предлагаю пока оставить, но держать в голове, что может когда-то выстрелить
There was a problem hiding this comment.
Может быть попробовать оптимизировать? Накладные расходы на работу с огромным количеством потоков (в пределе это n_cpu ** n_cpu) никто не отменял.
There was a problem hiding this comment.
Боюсь, что оптимизация не сильно поможет, так как алгоритмы сами по себе нелинейные. Но это лучше спросить у Ильи
There was a problem hiding this comment.
Посмотрел. Операции извлечения фич в основном линейные. Но проблема в том, что в процессе их генерации мы каждый лаг матрицы преобразуем в еще одну матрицу и работаем со второй матрицей, то есть сложность уже ближе к квадратичной (но не квадрат, поменьше)
There was a problem hiding this comment.
Не выйдет, так как не все операции векторизуются в контексте оптимизации времени. np.vectorise не ускоряет выполнение
There was a problem hiding this comment.
Я имел ввиду векторизацию в виде использования чистых numpy функций над одним многомерным np.ndarray.
Но тут надо разбираться как это можно реализовать.
There was a problem hiding this comment.
Мне кажется временные затраты >> выхлопа
There was a problem hiding this comment.
Тем более код не мой, я скорее его адаптирую для федота
There was a problem hiding this comment.
Выхлоп - экономия времени на каждом запуске топо внутри каждого пайплайна внутри каждого поколения для каждой оптимизации в каждом запуске федота. Выгода даже в 10% достаточна)) ИМХО.
Я могу помочь с адаптацией, но не в срочном порядке.
|
All PEP8 errors has been fixed, thanks ❤️ Comment last updated at |
|
/fix-pep8 |
| if not m4_id: | ||
| label = random.choice(np.unique(time_series['label'])) |
There was a problem hiding this comment.
Поставь ограничение на длину датасета, пожалуйста. Если он слишком короткий, то там много проблем с композицией пайплайнов возникает, которые корректно отрабатываются в проде, но падают на тесте. Я бы поставил длину ряда в районе 1000 точек - с ней все хорошо. С длиной до 400 очень часто падает.
There was a problem hiding this comment.
В тестах используются старые датасеты (beer, salaries и тд)
|
/fix-pep8 |
|
/fix-pep8 |
|
/fix-pep8 |
TopologicalFeaturesImplementation was added