Skip to content

fix: exclude index columns from model fitting processes. #1138

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Nov 12, 2024
Merged

fix: exclude index columns from model fitting processes. #1138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
854f48b
fix: exclude index columns from model fitting processes.
Genesis929 Nov 7, 2024
5b06d4b
update logic
Genesis929 Nov 7, 2024
e76824c
fix unit test
Genesis929 Nov 7, 2024
73a816b
remove empty line
Genesis929 Nov 7, 2024
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions bigframes/ml/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,9 +307,11 @@ def create_model(
# Cache dataframes to make sure base table is not a snapshot
# cached dataframe creates a full copy, never uses snapshot
if y_train is None:
input_data = X_train.cache()
input_data = X_train.reset_index(drop=True).cache()
Copy link
Contributor

Choose a reason for hiding this comment

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

To be consistent, we may also want to drop index for other "create_model" methods. Even though those don't matter (as they only select specific column(s) to fit.
else:
input_data = X_train.join(y_train, how="outer").cache()
input_data = (
X_train.join(y_train, how="outer").reset_index(drop=True).cache()
)
options.update({"INPUT_LABEL_COLS": y_train.columns.tolist()})

session = X_train._session
Expand Down
10 changes: 10 additions & 0 deletions tests/system/large/ml/test_cluster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,13 @@ def test_cluster_configure_fit_load_params(penguins_df_default_index, dataset_id
assert reloaded_model.distance_type == "COSINE"
assert reloaded_model.max_iter == 30
assert reloaded_model.tol == 0.001


def test_model_centroids_with_custom_index(penguins_df_default_index):
model = cluster.KMeans(n_clusters=3)
penguins = penguins_df_default_index.set_index(["species", "island", "sex"])
model.fit(penguins)

assert (
not model.cluster_centers_["feature"].isin(["species", "island", "sex"]).any()
)
27 changes: 27 additions & 0 deletions tests/system/large/ml/test_linear_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,3 +425,30 @@ def test_logistic_regression_customized_params_fit_score(
assert reloaded_model.tol == 0.02
assert reloaded_model.learning_rate_strategy == "CONSTANT"
assert reloaded_model.learning_rate == 0.2


def test_model_centroids_with_custom_index(penguins_df_default_index):
model = bigframes.ml.linear_model.LogisticRegression(
fit_intercept=False,
class_weight="balanced",
l2_reg=0.2,
tol=0.02,
l1_reg=0.2,
max_iterations=30,
optimize_strategy="batch_gradient_descent",
learning_rate_strategy="constant",
learning_rate=0.2,
)
df = penguins_df_default_index.dropna().set_index(["species", "island"])
X_train = df[
[
"culmen_length_mm",
"culmen_depth_mm",
"flipper_length_mm",
]
]
y_train = df[["sex"]]
model.fit(X_train, y_train)

# If this line executes without errors, the model has correctly ignored the custom index columns
model.predict(X_train.reset_index(drop=True))
19 changes: 15 additions & 4 deletions tests/unit/ml/test_golden_sql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,17 @@ def mock_X(mock_y, mock_session):
["index_column_id"],
["index_column_label"],
)

mock_X.join(mock_y).reset_index(drop=True).sql = "input_X_y_no_index_sql"
mock_X.join(mock_y).reset_index(drop=True).cache.return_value = mock_X.join(
mock_y
).reset_index(drop=True)
mock_X.join(mock_y).reset_index(drop=True)._to_sql_query.return_value = (
"input_X_y_no_index_sql",
["index_column_id"],
["index_column_label"],
)

mock_X.cache.return_value = mock_X

return mock_X
Expand All @@ -107,7 +118,7 @@ def test_linear_regression_default_fit(
model.fit(mock_X, mock_y)

mock_session._start_query_ml_ddl.assert_called_once_with(
"CREATE OR REPLACE MODEL `test-project`.`_anon123`.`temp_model_id`\nOPTIONS(\n model_type='LINEAR_REG',\n data_split_method='NO_SPLIT',\n optimize_strategy='auto_strategy',\n fit_intercept=True,\n l2_reg=0.0,\n max_iterations=20,\n learn_rate_strategy='line_search',\n min_rel_progress=0.01,\n calculate_p_values=False,\n enable_global_explain=False,\n INPUT_LABEL_COLS=['input_column_label'])\nAS input_X_y_sql"
"CREATE OR REPLACE MODEL `test-project`.`_anon123`.`temp_model_id`\nOPTIONS(\n model_type='LINEAR_REG',\n data_split_method='NO_SPLIT',\n optimize_strategy='auto_strategy',\n fit_intercept=True,\n l2_reg=0.0,\n max_iterations=20,\n learn_rate_strategy='line_search',\n min_rel_progress=0.01,\n calculate_p_values=False,\n enable_global_explain=False,\n INPUT_LABEL_COLS=['input_column_label'])\nAS input_X_y_no_index_sql"
)


Expand All @@ -117,7 +128,7 @@ def test_linear_regression_params_fit(bqml_model_factory, mock_session, mock_X,
model.fit(mock_X, mock_y)

mock_session._start_query_ml_ddl.assert_called_once_with(
"CREATE OR REPLACE MODEL `test-project`.`_anon123`.`temp_model_id`\nOPTIONS(\n model_type='LINEAR_REG',\n data_split_method='NO_SPLIT',\n optimize_strategy='auto_strategy',\n fit_intercept=False,\n l2_reg=0.0,\n max_iterations=20,\n learn_rate_strategy='line_search',\n min_rel_progress=0.01,\n calculate_p_values=False,\n enable_global_explain=False,\n INPUT_LABEL_COLS=['input_column_label'])\nAS input_X_y_sql"
"CREATE OR REPLACE MODEL `test-project`.`_anon123`.`temp_model_id`\nOPTIONS(\n model_type='LINEAR_REG',\n data_split_method='NO_SPLIT',\n optimize_strategy='auto_strategy',\n fit_intercept=False,\n l2_reg=0.0,\n max_iterations=20,\n learn_rate_strategy='line_search',\n min_rel_progress=0.01,\n calculate_p_values=False,\n enable_global_explain=False,\n INPUT_LABEL_COLS=['input_column_label'])\nAS input_X_y_no_index_sql"
)


Expand Down Expand Up @@ -150,7 +161,7 @@ def test_logistic_regression_default_fit(
model.fit(mock_X, mock_y)

mock_session._start_query_ml_ddl.assert_called_once_with(
"CREATE OR REPLACE MODEL `test-project`.`_anon123`.`temp_model_id`\nOPTIONS(\n model_type='LOGISTIC_REG',\n data_split_method='NO_SPLIT',\n fit_intercept=True,\n auto_class_weights=False,\n optimize_strategy='auto_strategy',\n l2_reg=0.0,\n max_iterations=20,\n learn_rate_strategy='line_search',\n min_rel_progress=0.01,\n calculate_p_values=False,\n enable_global_explain=False,\n INPUT_LABEL_COLS=['input_column_label'])\nAS input_X_y_sql"
"CREATE OR REPLACE MODEL `test-project`.`_anon123`.`temp_model_id`\nOPTIONS(\n model_type='LOGISTIC_REG',\n data_split_method='NO_SPLIT',\n fit_intercept=True,\n auto_class_weights=False,\n optimize_strategy='auto_strategy',\n l2_reg=0.0,\n max_iterations=20,\n learn_rate_strategy='line_search',\n min_rel_progress=0.01,\n calculate_p_values=False,\n enable_global_explain=False,\n INPUT_LABEL_COLS=['input_column_label'])\nAS input_X_y_no_index_sql"
)


Expand All @@ -172,7 +183,7 @@ def test_logistic_regression_params_fit(
model.fit(mock_X, mock_y)

mock_session._start_query_ml_ddl.assert_called_once_with(
"CREATE OR REPLACE MODEL `test-project`.`_anon123`.`temp_model_id`\nOPTIONS(\n model_type='LOGISTIC_REG',\n data_split_method='NO_SPLIT',\n fit_intercept=False,\n auto_class_weights=True,\n optimize_strategy='batch_gradient_descent',\n l2_reg=0.2,\n max_iterations=30,\n learn_rate_strategy='constant',\n min_rel_progress=0.02,\n calculate_p_values=False,\n enable_global_explain=False,\n l1_reg=0.2,\n learn_rate=0.2,\n INPUT_LABEL_COLS=['input_column_label'])\nAS input_X_y_sql"
"CREATE OR REPLACE MODEL `test-project`.`_anon123`.`temp_model_id`\nOPTIONS(\n model_type='LOGISTIC_REG',\n data_split_method='NO_SPLIT',\n fit_intercept=False,\n auto_class_weights=True,\n optimize_strategy='batch_gradient_descent',\n l2_reg=0.2,\n max_iterations=30,\n learn_rate_strategy='constant',\n min_rel_progress=0.02,\n calculate_p_values=False,\n enable_global_explain=False,\n l1_reg=0.2,\n learn_rate=0.2,\n INPUT_LABEL_COLS=['input_column_label'])\nAS input_X_y_no_index_sql"
)


Expand Down