-
Notifications
You must be signed in to change notification settings - Fork 87
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
922 preprocessor acceleration #1004
Conversation
fedot/preprocessing/categorical.py
Outdated
nans_number = is_row_has_nan.sum() | ||
if nans_number > 0 and column_id in categorical_ids: | ||
for column_id in range(number_of_columns): | ||
pd_column = pd.Series(input_data.features[:, column_id], copy=True) |
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.
А оно и раньше было - column = np.array(input_data.features[:, column_id])
Ну и потом, без этого копирования тесты падают, мб можно от этого копирования избавиться, но не придумал ещё, как.
Вот тут скрываются пруфы, почему буст результата не дал |
fedot/preprocessing/data_types.py
Outdated
@@ -412,58 +412,42 @@ def _into_numeric_features_transformation_for_predict(self, data: InputData): | |||
features_types[column_id] = NAME_CLASS_FLOAT | |||
|
|||
|
|||
def define_column_types(table: np.array): | |||
def define_column_types(table: np.ndarray): |
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.
А есть тут adult_medium.zip
fedot/preprocessing/data_types.py
Outdated
for column_id in range(n_columns): | ||
current_column = table[:, column_id] | ||
|
||
# Check every element in numpy array - it can take a long time! | ||
column_types = list(map(type_ignoring_nans, current_column)) | ||
column_types = np.where(pd.isna(current_column), str(type(None)), vto_type(current_column)) | ||
unique_column_types = np.unique(column_types) | ||
|
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.
Вот как это выглядит с такой правкой:
Вот файл adult_medium_improved.zip
Получается, что теперь время на векторизацию вообще почти не тратится, с другой стороны, unique метод стал почему-то сразу больше времени занимать ни с того ни с сего.
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.
я имел ввиду np.unique прогнать один раз: np.unique(... , axis=1)
, кроме того можно сразу один раз посчитать количество элементов всех типов np.unique(..., return_counts=True)
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.
еще пара мыслей:
- вместо строчек-имен типов может быть эффективнее хранить численные id типов или их int хэш
- вместо сложной логики подмены nan и писаиваний в массив может быть имеет смысл просто вычесть из числа float-типов число nan значений
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.
я имел ввиду np.unique прогнать один раз:
np.unique(... , axis=1)
, кроме того можно сразу один раз посчитать количество элементов всех типовnp.unique(..., return_counts=True)
Не получится axis
задать для dtype=object
, поэтому всё ещё внутри цикла это происходит.
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.
еще пара мыслей:
- вместо строчек-имен типов может быть эффективнее хранить численные id типов или их int хэш
- вместо сложной логики подмены nan и писаиваний в массив может быть имеет смысл просто вычесть из числа float-типов число nan значений
С первым пунктом согласен, так и сделал, создав словарь TYPE_TO_ID
.
А вот второй пункт вообще, если честно, не понял.
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.
тогда, может, и комментарий выше насчет np.unique сработает для dtype=int ?
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.
А вот второй пункт вообще, если честно, не понял.
Я имел ввиду следующее. количество не-nan-значений можно посчитать как (количество всех float-значений) - (количество nan). Это сработает так как nan-значение имеет тип float. А кол-во nan можно посчитать с помощью np.isna. Так можно обойтись без поиндексного присваивания в массив и сравниваний значений типа object -- а это все заметно дольше.
5aa9531
to
cd5a712
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1004 +/- ##
==========================================
- Coverage 79.27% 79.08% -0.20%
==========================================
Files 145 145
Lines 10047 9957 -90
==========================================
- Hits 7965 7874 -91
- Misses 2082 2083 +1 ☔ View full report in Codecov by Sentry. |
6da30b3
to
f3befb2
Compare
bc38230
to
8f05890
Compare
bc38230
to
4a97956
Compare
Hello @IIaKyJIuH! 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-12-11 16:52:39 UTC |
29d27a2
to
a346643
Compare
9e4cff2
to
c48da70
Compare
c48da70
to
8e86b98
Compare
ada3c49
to
177cd28
Compare
299bce9
to
d75a7ab
Compare
9f39d7b
to
4b17129
Compare
@IIaKyJIuH
А |
3745685
to
8c793bb
Compare
need_label = True | ||
break | ||
return need_label | ||
uniques = np.unique(input_data.features[:, categorical_ids].astype(str)) |
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.
- Зачем приводить к
str
? - В предыдущем варианте учитывалось, что в разных столбцах могут быть одинаковые значения. В новом варианте это не учитывается, хотя по логике должно.
- Если отказаться от приведения к
str
, то надо будет добавить аргументequal_nan
, это должно быть быстрее, чем приводить кstr
.
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.
Зачем приводить к str?
Так это же категориальные признаки. Думаю, что там могут быть и числа, например, 1, 2, 3 и тд. Наверное, в этом и была идея переводить к str
В предыдущем варианте учитывалось, что в разных столбцах могут быть одинаковые значения. В новом варианте это не учитывается, хотя по логике должно. Если отказаться от приведения к str, то надо будет добавить аргумент equal_nan, это должно быть быстрее, чем приводить к str.
Не понял. Можешь более детальнее?
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.
Так это же категориальные признаки. Думаю, что там могут быть и числа, например, 1, 2, 3 и тд. Наверное, в этом и была идея переводить к str
Странно, потому что '1'
и 1
в таком случае получатся одной категорией.
Не понял. Можешь более детальнее?
Для нового кода 1
в первом столбце и 1
в любом другом - это одна уникальная категория. В старом коде 1
в первом столбце - это уникальное значение для первого столбца, а 1
во втором - для второго.
@@ -118,11 +115,5 @@ def control_categorical(self, input_data: InputData) -> bool: | |||
""" | |||
|
|||
categorical_ids, _ = find_categorical_columns(input_data.features) |
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.
Поиск категориальных столбцов требует времени, поэтому лучше брать индексы категориальных столбцов из input_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.
Соглашусь с тобой. Думаю, что можно было бы проследить вызовы этой функции. Сохранение сделано для извлечения незакодированных категориальных признаков, добавлял такой признак в InputData
, который сохраняет на одном из этапов предобработки.
Однако думаю, что это мог бы быть оформлен в виде issue и выполнен последующим шагом, а не в этом PR.
if isinstance(self.train_data, InputData) and self.params.get('use_auto_preprocessing'): | ||
self.train_data = self.data_processor.fit_transform(self.train_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.
А почему только для InputData
?
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.
Сделал так, потому что в MultiModal
данных отсутствует supplementary_data
. Из-за этого падали тесты. Думаю, что для них нужно сделать как-то по другому, и авто предобработать только если в них содержатся табличные данные. Пока не знаю как это можно лучше всего это сделать.
if not feature_ids: | ||
return 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.
Думаю, что None
не просто так. Если посмотреть на использование функции, то от нее ожидается такое поведение. Если таких индексов нет, например, категориальных, то и данные должны быть пустыми, то есть None
.
def data_type_is_suitable_for_preprocessing(data: InputData) -> bool: | ||
return data_type_is_table(data) or data_type_is_ts(data) or data_type_is_multi_ts(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.
Зачем нужны эти функции если можно через data_type in List[DataTypesEnum]
?
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.
Не знаю, это уже было давно так сделано
need_label = True | ||
break | ||
return need_label | ||
uniques = np.unique(input_data.features[:, categorical_ids].astype(str)) |
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.
Так это же категориальные признаки. Думаю, что там могут быть и числа, например, 1, 2, 3 и тд. Наверное, в этом и была идея переводить к str
Странно, потому что '1'
и 1
в таком случае получатся одной категорией.
Не понял. Можешь более детальнее?
Для нового кода 1
в первом столбце и 1
в любом другом - это одна уникальная категория. В старом коде 1
в первом столбце - это уникальное значение для первого столбца, а 1
во втором - для второго.
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.
Я бы сделал флаг true по умолчанию. Потому что иначе никто не станет им пользоваться и мы не узнаем насколько изменения хороши в бою
@valer1435 Кажется нам нужно сначала самим также ее протестировать. Хоть я и постарался покрыть код тестами, но хотелось бы увидеть и на качественные изменения, если они есть. Вливаю в мастер, так как сейчас есть актуальные эксперименты с необходимостью такого функционала, на которых и посмотрим. Далее, если нужно будет доработать, внесем изменения и поставим по умолчанию. |
@@ -131,3 +132,13 @@ def df_to_html(df: pd.DataFrame, save_path: Union[str, os.PathLike], name: str = | |||
if table.parent.name != 'div': | |||
table = table.wrap(doc.new_tag('div', style='overflow: auto;')) | |||
file.write_text(doc.prettify()) | |||
|
|||
|
|||
def convert_memory_size(size_bytes): |
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/preprocessing/preprocessing.py
Outdated
try: | ||
return item.strip() | ||
except AttributeError: | ||
# not an str object |
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.
Поправь на "not a str object"
Fixes to increase preprocessor speed.
Optimized preprocessing directory of the project as well as some core subdirectories with preprocessing utils.