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

Adiutant · 2024-12-21T19:33:51Z

Description

Set default model name in Column.Model column, when using VisualApp #118
Closes #118

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Optimization

How Has This Been Tested?

Before submitting a PR, please check yourself against the following list. It would save us quite a lot of time.

Have you read the contribution guide?
Have you updated the relevant docstrings? We're using Numpy format, please double-check yourself
Does your change require any new tests?
Have you updated the changelog file?

blondered

Hi! Thank you so much for you contribution. I have suggested just a few tiny things to improve.

tests/visuals/test_visual_app.py

blondered · 2024-12-24T14:26:17Z

tests/visuals/test_visual_app.py

-                is_u2i=True,
-                selected_requests=SELECTED_REQUESTS_U2I,
-            )
+        incorrect_reco = pd.DataFrame(


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

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

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.

rectools/visuals/visual_app.py

CHANGELOG.md

codecov · 2025-01-10T09:15:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (9b3992e) to head (3dc45d6).
Report is 86 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##              main      #235     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files           45        59     +14     
  Lines         2242      3877   +1635     
===========================================
+ Hits          2242      3877   +1635

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

blondered · 2025-01-10T09:09:56Z

rectools/visuals/visual_app.py

@@ -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


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

blondered · 2025-01-10T09:19:05Z

tests/visuals/test_visual_app.py

@@ -21,7 +21,12 @@
 import pytest

 from rectools import Columns, ExternalId
-from rectools.visuals.visual_app import AppDataStorage, ItemToItemVisualApp, StorageFiles, TablesDict, VisualApp
+from rectools.visuals.visual_app import (AppDataStorage,


Please run make format command in your terminal before pushing your changes. It will run isort and black to fix code style.
Also it is useful to run make lint command to check that all linters are passing locally. Right now they are failing because of unusual formatting.

blondered · 2025-01-10T09:21:37Z

tests/visuals/test_visual_app.py

+                )
+            }
+        }
+        assert expected_grouped_reco.keys() == ads.grouped_reco.keys()


Great to see this added to the test!

Could you please move this block of code (lines 256-26) to a separate method of the class? You can call it assert_equal_data_in_grouped_reco_and_ads.
Right now we are copy-pasting this line 3 times in the file and it's too much.

blondered · 2025-01-10T09:25:01Z

tests/visuals/test_visual_app.py

-                is_u2i=True,
-                selected_requests=SELECTED_REQUESTS_U2I,
-            )
+        incorrect_reco = pd.DataFrame(


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.

feldlime · 2025-01-10T14:46:50Z

rectools/visuals/visual_app.py

@@ -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


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")

Roman Legonkov added 3 commits December 21, 2024 22:21

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

c3a8642

update changelog

424ef0d

update changelog

1af5f3a

blondered reviewed Dec 24, 2024

View reviewed changes

review changes

3dc45d6

Adiutant requested a review from blondered December 24, 2024 20:02

blondered reviewed Jan 10, 2025

View reviewed changes

fixed imports

c9814c9

feldlime reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Adiutant commented Dec 21, 2024 •

edited by blondered

Loading

blondered left a comment

blondered Dec 24, 2024

Adiutant Dec 24, 2024 •

edited

Loading

blondered Jan 10, 2025

codecov bot commented Jan 10, 2025

blondered Jan 10, 2025

blondered Jan 10, 2025

blondered Jan 10, 2025

blondered Jan 10, 2025

feldlime Jan 10, 2025 •

edited

Loading

	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

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

Are you sure you want to change the base?

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

Conversation

Adiutant commented Dec 21, 2024 • edited by blondered Loading

Description

Type of change

How Has This Been Tested?

blondered left a comment

Choose a reason for hiding this comment

blondered Dec 24, 2024

Choose a reason for hiding this comment

Adiutant Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

blondered Jan 10, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2025

Codecov Report

blondered Jan 10, 2025

Choose a reason for hiding this comment

blondered Jan 10, 2025

Choose a reason for hiding this comment

blondered Jan 10, 2025

Choose a reason for hiding this comment

blondered Jan 10, 2025

Choose a reason for hiding this comment

feldlime Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Adiutant commented Dec 21, 2024 •

edited by blondered

Loading

Adiutant Dec 24, 2024 •

edited

Loading

feldlime Jan 10, 2025 •

edited

Loading