Skip to content
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

Jump to bottom

LTR notebook #177

Merged
merged 19 commits into from
Feb 1, 2024
Merged

LTR notebook #177

merged 19 commits into from
Feb 1, 2024

Conversation

afoucret
Copy link
Contributor

@afoucret afoucret commented Jan 29, 2024 •
edited
Loading

TODO:

  • Add some intro text:
    • Be clear with the tech preview status of the feature
    • Set min version requirement to 8.12.1
  • Update to eland 8.12.1
  • Before merging, clean all the link to the fork

@joshdevins joshdevins self-requested a review January 29, 2024 11:05
joshdevins
joshdevins requested changes Jan 29, 2024
View reviewed changes
Copy link
Member

@joshdevins joshdevins left a comment

Choose a reason for hiding this comment

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

Since this is the first and most basic LTR notebook, I'd actually prefer to see it as part of the search notebooks. It feels to me like it belongs there as part of the user journey from basic search to advanced, as shown through the search notebooks.

notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/sample_data/movies_index_settings.json Outdated Show resolved Hide resolved
notebooks/learning-to-rank/sample_data/movies_index_settings.json Outdated
"id": {"type": "keyword"},
"title": {"type": "text", "analyzer": "english"},
"overview": {"type": "text", "analyzer": "english"},
"year": {"type": "integer"},
Copy link
Member

Choose a reason for hiding this comment

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

Is this better as a date field? Not sure how it's being used.

notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated
"\n",
"TODO: udpate the link to elastic/elasticsearch-labs instead of my fork before merging.\n",
"\n",
"[![Open In Colab](https://colab.research.google.com/assets/colab-badge.svg)](https://colab.research.google.com/github/afoucret/elasticsearch-labs/blob/ltr-notebook/notebooks/learning-to-rank/01-learning-to-rank.ipynb)\n",
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to change this before merging 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
maxjakob
maxjakob reviewed Jan 29, 2024
View reviewed changes
Copy link
Contributor

@maxjakob maxjakob left a comment

Choose a reason for hiding this comment

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

I like it. Added lots of small suggestions

notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/sample_data/movies_judgments.csv.gz Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
maxjakob
maxjakob approved these changes Jan 30, 2024
View reviewed changes
Copy link
Contributor

@maxjakob maxjakob 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 applying all the changes @afoucret! I added a handful of extra suggestions, but for me this looks good once the TODOs in the description are done 👍

notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
notebooks/learning-to-rank/01-learning-to-rank.ipynb Outdated Show resolved Hide resolved
@afoucret afoucret marked this pull request as ready for review February 1, 2024 13:43
@afoucret afoucret merged commit ea994c3 into elastic:main Feb 1, 2024
2 checks passed
maxjakob
maxjakob reviewed Feb 5, 2024
View reviewed changes
notebooks/search/08-learning-to-rank.ipynb
"cell_type": "markdown",
"metadata": {},
"source": [
"We saw above that the title and popularity fields are important ranking feature in our model. Here we can see that now all results contain the query terms in the title. Moreover, more popular movies rank higher, for example `Star Wars: Episode I - The Phantom Menace` is now in third position."
Copy link
Contributor

Choose a reason for hiding this comment

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

@afoucret I saw that in the version that got merged Star Wars: Episode I - The Phantom Menace is actually not in the result list. I think it would be good to

  • update the example with a movie that is actually here
  • set random.seed to a fixed number early in the notebook so that parts that use random values always do the same for each run.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxjakob maxjakob maxjakob approved these changes

@joshdevins joshdevins joshdevins approved these changes

Assignees

@afoucret afoucret

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@afoucret @joshdevins @maxjakob