Skip to content

[RLlib] RecSim Interest evolution environment should use custom video sampler: IEvVideoSampler due to only one cluster being used.#22211

Merged
sven1977 merged 3 commits intoray-project:masterfrom
gjoliver:iev_video_sampler
Feb 9, 2022
Merged

Conversation

@gjoliver
Copy link
Member

@gjoliver gjoliver commented Feb 8, 2022

Why are these changes needed?

Interest evolution env should probably use IEV video sampler, instead of the utility model video sampler.
Docs returned from this sampler contains actual features, instead of utility indices.
This may help your slateq runs.

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Docs returned from this sampler contains actual features, instead of utility indices.
@gjoliver gjoliver requested a review from sven1977 February 8, 2022 09:25
@gjoliver gjoliver requested a review from avnishn as a code owner February 8, 2022 09:25
@sven1977 sven1977 self-assigned this Feb 8, 2022
Copy link
Contributor

@sven1977 sven1977 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @gjoliver !

@sven1977
Copy link
Contributor

sven1977 commented Feb 8, 2022

Waiting for LINT to pass.

return iev.IEvUserModel(
env_ctx["slate_size"],
choice_model_ctor=choice_model.MultinomialProportionalChoiceModel,
choice_model_ctor=choice_model.MultinomialLogitChoiceModel,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference and why did we need to change this? In the original RecSim repo, they use:
choice_model.MultinomialProportionalChoiceModel.

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference is how to handle negative logits.

MultinomialLogitChoiceModel uses p(x) = exp(x) / Sum_{y in scores} exp(y), while
MultinomialProportionalChoiceModel uses p(x) = (x - min_normalizer) / sum(x - min_normalizer). You need to know the lower bound of your output logits before you can convert everything to be positive.
intuitively, these 2 should work similarly, like the more negative model output is, the less likely it will get clicked on.

@sven1977 sven1977 changed the title Interest evolution environment should probably use IEvVideoSampler. [RLlib] RecSim Interest evolution environment should use custom video sampler: IEvVideoSampler. Feb 8, 2022
@sven1977 sven1977 changed the title [RLlib] RecSim Interest evolution environment should use custom video sampler: IEvVideoSampler. [RLlib] RecSim Interest evolution environment should use custom video sampler: IEvVideoSampler due to only one cluster being used. Feb 8, 2022
@gjoliver
Copy link
Member Author

gjoliver commented Feb 8, 2022

I actually noticed another issue with RecSim.

IEvUserModel is hardcoded to use UtilityModelUserSampler:
https://github.com/google-research/recsim/blob/55e50e4be736d222ffe8c2477ed1981b40f91605/recsim/environments/interest_evolution.py#L491-L492

So I don't know if switching to IEvVideoSampler actually works or not ...

@sven1977 sven1977 merged commit 3207f53 into ray-project:master Feb 9, 2022
simonsays1980 pushed a commit to simonsays1980/ray that referenced this pull request Feb 27, 2022
… sampler: `IEvVideoSampler` due to only one cluster being used. (ray-project#22211)
simonsays1980 pushed a commit to simonsays1980/ray that referenced this pull request Mar 8, 2022
… sampler: `IEvVideoSampler` due to only one cluster being used. (ray-project#22211)
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