Skip to content

Fix division by zero in WeightedRandomStreamRanker #16

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
eltongo wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix division by zero in WeightedRandomStreamRanker #16

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0ec271d
Fix division by zero in WeightedRandomStreamRanker
Aug 10, 2023
038a543
Add braces to if statement
Aug 10, 2023
e338b3a
Merge branch 'main' into elton/weighted-ranker-divbyzero
eltongo Aug 10, 2023
a29a1ea
Merge branch 'main' into elton/weighted-ranker-divbyzero
lucila Aug 17, 2023
c877a55
dont store 2 elements on the same key if the key is the same
Aug 17, 2023
e5b83ef
fix test
Aug 17, 2023
fa345b1
Merge branch 'main' into elton/weighted-ranker-divbyzero
rtio Jun 17, 2024
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
8 changes: 6 additions & 2 deletions lib/Tumblr/StreamBuilder/StreamRankers/WeightedRandomStreamRanker.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,12 @@ protected function rank_inner(array $stream_elements, StreamTracer $tracer = nul
foreach ($valid_elements as $element) {
/** @var RecommendationLeafStreamElementTrait $original_element */
$original_element = $element->get_original_element();
// calculate sampling score
$r = pow(mt_rand() / $max_rand, (1 / $original_element->get_score()));
if ($original_element->get_score() === 0.0) {

@lovemeblender lovemeblender Aug 10, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

just as a note, if for any reason this comes as 0 instead, 0===0.0 evaluates to false

$r = 0;
} else {
// calculate sampling score
$r = pow(mt_rand() / $max_rand, (1 / $original_element->get_score()));
}
$H[strval($r)] = $element;

@eltongo eltongo Aug 10, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't know if we need to be worried about this line though: $H[strval($r)] = $element;. An element with a score of zero should be a rare occurrence, but is it possible some elements will be lost? It seems that way?

@cyle cyle Aug 10, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i feel like we should have some logging somewhere to show us definitively whether a score of 0 actually happens... maybe we should add some here, if not?

@cyle cyle Aug 10, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok, i see that we do have this logging, since the exception is happening in production. in that case, maybe a decision could be made here to just drop the element entirely if the weight ends up being 0. that seems to make sense to me? cc @nicola-barbieri @lucila i guess it's a separate broader question of what we should do here when two elements have the exact same score, because yeah, they'll collide here.

@eltongo eltongo Aug 10, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it might still be possible for two elements to have the same score, even if not zero though

@lovemeblender lovemeblender Aug 10, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When score->0 then r->infinity, in which case I think assigning zero to it is far from the actual final score.

It all depends on the requirements of this ranker and what a score=0 means:

  1. if it's an error it means nothing and we can give it indeed a very low score or even make this element an invalid one (you can do this below)
  2. If it's not an error and it's an acceptable value which is just not handled then r should be a high number and not zero or invalid.

@eltongo eltongo Aug 10, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so what if, theoretically, we get two elements with the same score. Only one of those will end up in $H. Does this mean the other one gets discarded?

@lucila lucila Aug 10, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

another alternative I'm thinking about, to reduce the chances of having more than 1 element with the same score, is to do something like:

$score = $original_element->get_score();
if ($score === 0) {
  $score = 0.001;
}
$r = pow(mt_rand() / $max_rand, (1 / $score));

so that we will still call mt_rand() with a very small value on the exp part.

thoughts? @eltongo

I think this is an alternative @lovemeblender had suggested before (internally on a huddle)

@eltongo eltongo Aug 10, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would think it is also possible for two elements to have the same non-zero score, no? In that case, what we need is to fix $H, because this would only fix the case for the score being zero.

@lovemeblender lovemeblender Aug 10, 2023 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

mt_rand() <= $max_rand

if that's true we're set with either zero or adding an very small number to avoid collisions ^ collisions will be handled gracefully I think as the elements will just appear adjacently

@nicola-barbieri nicola-barbieri Aug 11, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with fixing the division by zero, maybe adding an epsilon to all input scores:
$score = $score + $epsilon (0.00001)

score = 0 shouldn't happen in the first place (unless rounding errors). which element is causing this ?

}
// sort by key in descending order
Expand Down
4 changes: 4 additions & 0 deletions tests/unit/Tumblr/StreamBuilder/StreamRankers/WeightedRandomStreamRankerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,16 @@ public function test_ranker_with_stream_elements(): void
1234 => 1.0,
2345 => 2.0,
3456 => 3.0,
4567 => 0.0,
];
$stream_elements = $this->build_blog_stream_elements($bid2score);
mt_srand(0);
$ranked_elements = $this->ranker->rank($stream_elements);
mt_srand(0);
$weighted_score = array_map(function ($score) {
if ($score === 0.0) {
return 0;
}
return pow(mt_rand() / mt_getrandmax(), (1 / $score));
}, $bid2score);
// sort by value in descending order
Expand Down