-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix: proper retrieval of answers for batch eval #3245
Conversation
@bglearning and @masci let me know what you guys think. Would you also please run the existing manual tests that fail? |
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 think that piece of code should be refactored a bit but for the scope of this PR it's better to keep the diff small, so it's 👍 from my side.
I tested your branch on Tutorial 15 in colab and it works.
@vblagoje Just realized the error that Miriam had was from a different line which has a similar faulty pattern. Specifically this one base.py#L1610 if documents is not None:
if isinstance(documents[i], list): # Index out of range with this
documents = documents[i]
if len(documents) == 0:
... Maybe best to also update that line too. |
@bglearning added your fix, please verify. If all is good, please integrate as well. |
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.
LGTM 👍
* Proper retrieval of answers and documents for batch eval
Related Issues
Proposed Changes:
Improper list access. Fixed the list access
How did you test it?
TODO
Notes for the reviewer
Please review carefully and run the previous failing tests
Checklist