Skip to content

Conversation

hane1818
Copy link

Fixes #[include a number of issue this PR is fixing].

Summary of changes proposed in this Pull Request:

  • Add a simple ranking model

PR checklist:

  • Updated relevant documentation
  • Updated CHANGELOG.md
  • Added tests for my change

from recommendation_system.ranking_layer.base import BaseRankingModel

class SimpleRankingModel(BaseRankingModel):
def __init__(self) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

thx for annotating mypy typing!

"ranking_model": "demo",
"ranking_model": "simple",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice! we have ranking right now~

Comment on lines +10 to +12
def rank(self, user_features: Dict, candidates: List[List[Dict]], top_k: int) -> List[Dict]:
candidates = self.process_data(candidates=candidates)
rankings = self.calc_candidate_score(candidates=candidates)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice, very clear!

def get_item(self, item):
return deepcopy(self._items_mapping[item])

def calc_candidate_score(self, candidates):
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
def calc_candidate_score(self, candidates):
def _calc_candidate_score(self, candidates):


return sorted(candidate_ranking.items(), key=lambda d: d[1], reverse=True)

def process_data(self, candidates: List[List[Dict]]) -> List[List[Tuple]]:
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
def process_data(self, candidates: List[List[Dict]]) -> List[List[Tuple]]:
def _process_data(self, candidates: List[List[Dict]]) -> List[List[Tuple]]:

return sorted(candidate_ranking.items(), key=lambda d: d[1], reverse=True)

def process_data(self, candidates: List[List[Dict]]) -> List[List[Tuple]]:
items_mapping = {}
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
items_mapping = {}
items_mapping = defaultdict(dict)

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, would you add typing for items_mapping? seems to me it's the key data structure of your algorithm. Might be worth annotating~

items_mapping = {}
new_candidates = []

for ranker in candidates:
Copy link
Collaborator

Choose a reason for hiding this comment

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

why ranker is in candidates?
I'm not sure if I understand what does this ranker stand for 😅

Comment on lines +16 to +17
new_item = self.get_item(item)
new_item['score'] = score
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you'd calculated score in calc_candiate_score func, then maybe you can return item along with score field right? thoughts?

Copy link
Collaborator

@david30907d david30907d left a comment

Choose a reason for hiding this comment

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

overall LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants