Skip to content

if column.model not provided in visual app then set default model name #235

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

if column.model not provided in visual app then set default model name #235

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c3a8642
if column.model not provided in visual app then set default model name
Dec 21, 2024
424ef0d
update changelog
Dec 21, 2024
1af5f3a
update changelog
Dec 21, 2024
3dc45d6
review changes
Dec 24, 2024
c9814c9
fixed imports
Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

### Added
- If `Column.Model` is not provided in `VisualApp`, then default model name added to dataframe ([#235](https://github.com/MobileTeleSystems/RecTools/pull/235))
- Changed tests for `VisualApp` missing columns

## [0.9.0] - 11.12.2024

Expand Down
6 changes: 4 additions & 2 deletions rectools/visuals/visual_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
MIN_WIDTH_LIMIT = 10
REQUEST_NAMES_COL = "request_name"
REQUEST_IDS_COL = "request_id"
DEFAULT_MODEL_NAME = "model1"

VisualAppT = tp.TypeVar("VisualAppT", bound="VisualAppBase")

Expand Down Expand Up @@ -71,7 +72,8 @@ def from_raw(
----------
reco : tp.Union[pd.DataFrame, TablesDict]
Recommendations from different models in a form of a pd.DataFrame or a dict.
In DataFrame form model names must be specified in `Columns.Model` column. In dict form
In DataFrame form model names must be specified in `Columns.Model` column.
If not, `Columns.Model` column will be created with default value ``model1``. In dict form
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
If not, `Columns.Model` column will be created with default value ``model1``. In dict form
If not, `Columns.Model` column will be created with default value ``model``. In dict form

model names are supposed to be dict keys.
item_data : pd.DataFrame
Data for items that is used for visualisation in both interactions and recommendations
Expand Down Expand Up @@ -100,7 +102,7 @@ def from_raw(

if isinstance(reco, pd.DataFrame):
if Columns.Model not in reco.columns:
raise KeyError("Missing `{Columns.Model}` column in `reco` DataFrame")
reco[Columns.Model] = DEFAULT_MODEL_NAME
Copy link
Collaborator

@feldlime feldlime Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, please add a warning here

warnings.warn("`{Columns.Model}` column in `reco` DataFrame is missing. All recommendations will be related to a single model")

reco = cls._df_to_tables_dict(reco, Columns.Model)
cls._check_columns_present_in_reco(reco=reco, id_col=id_col)

Expand Down
24 changes: 12 additions & 12 deletions tests/visuals/test_visual_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
INTERACTIONS = pd.DataFrame({Columns.User: [1, 1, 2], Columns.Item: [3, 7, 8]})
SELECTED_REQUESTS_U2I: tp.Dict[tp.Hashable, tp.Hashable] = {"user_one": 1, "user_three": 3}
SELECTED_REQUESTS_I2I: tp.Dict[tp.Hashable, tp.Hashable] = {"item_three": 3}

DEFAULT_MODEL_NAME = "model1"

def check_data_storages_equal(one: AppDataStorage, two: AppDataStorage) -> None:
assert one.id_col == two.id_col
Expand Down Expand Up @@ -229,17 +229,17 @@ def test_missing_columns_validation(self) -> None:
)

# Missing `Columns.Model` in reco pd.DataFrame
with pytest.raises(KeyError):
incorrect_reco = pd.DataFrame(
{Columns.User: [1, 2, 3, 4], Columns.Item: [3, 4, 3, 4], Columns.Score: [0.99, 0.9, 0.5, 0.5]}
)
AppDataStorage.from_raw(
reco=incorrect_reco,
item_data=ITEM_DATA,
interactions=INTERACTIONS,
is_u2i=True,
selected_requests=SELECTED_REQUESTS_U2I,
)
incorrect_reco = pd.DataFrame(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move this to a separate test with test_happy_path_with_missing_model.
And please add assertion on expected_grouped_reco data.

Copy link
Author

@Adiutant Adiutant Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, there is my suggestion, how to name this test - test_successful_path_with_missing_model. Added assertion on expected_grouped_reco

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great! Could you please name it happy? It's just our common naming everywhere in the tests. This way it'a a bit easier to navigate in the code.

I also added a few more comments. And we will be ready to merge.

{Columns.User: [1, 2, 3, 4], Columns.Item: [3, 4, 3, 4], Columns.Score: [0.99, 0.9, 0.5, 0.5]}
)
ads = AppDataStorage.from_raw(
reco=incorrect_reco,
item_data=ITEM_DATA,
interactions=INTERACTIONS,
is_u2i=True,
selected_requests=SELECTED_REQUESTS_U2I,
)
assert "model1" in ads.model_names

def test_incorrect_interactions_for_reco_case(self) -> None:

Expand Down