-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add ranking evaluation API to High Level Rest Client #28357
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened a separate PR (#28341) to adress this, so depending on which one gets in first this might already be irrelevant.
javanna
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple of questions, LGTM though. Good stuff ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do we do here with random fields, forward comp etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added inserting random fields, which of course exposed some issues. Also fixed those in the next commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, should we test random fields insertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasn't this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, the fields are already set as a ctor argument before (L548), so this is redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool thanks
1da7dfd to
19a4254
Compare
603052c to
aaad9a1
Compare
This change adds support for the new ranking evaluation API to the High Level Rest Client. This mostly means adding support for parsing the various response objects back from the REST representation. It includes one change to the response syntax where previously we didn't print the type of the metric details section but we now need it to pick the right parser to parse this section back. Closes #28198
This change adds support for the new ranking evaluation API to the High Level Rest Client.
This mostly means adding support for parsing the various response objects back from the
REST representation. It includes one change to the response syntax where previously we didn't
print the type of the metric details section but we now need it to pick the right parser to
parse this section back. Since this API is marked as experimental this should be okay to change
on a minor version.