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.

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

Merged
merged 2 commits into from
Sep 20, 2022
Merged

fix: proper retrieval of answers for batch eval #3245

merged 2 commits into from
Sep 20, 2022

Conversation

vblagoje
Copy link
Member

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

@vblagoje vblagoje requested a review from a team as a code owner September 19, 2022 17:29
@vblagoje vblagoje requested review from bogdankostic, bglearning and masci and removed request for a team and bogdankostic September 19, 2022 17:29
@vblagoje
Copy link
Member Author

@bglearning and @masci let me know what you guys think. Would you also please run the existing manual tests that fail?

Copy link
Contributor

@masci masci left a 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.

@bglearning
Copy link
Contributor

@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.

@vblagoje
Copy link
Member Author

@bglearning added your fix, please verify. If all is good, please integrate as well.

Copy link
Contributor

@bglearning bglearning left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@vblagoje vblagoje merged commit fe31896 into deepset-ai:main Sep 20, 2022
@vblagoje vblagoje self-assigned this Sep 20, 2022
@masci masci changed the title Proper retrieval of answers for batch eval fix: proper retrieval of answers for batch eval Sep 21, 2022
brandenchan pushed a commit that referenced this pull request Sep 21, 2022
* Proper retrieval of answers and documents for batch eval
@vblagoje vblagoje deleted the fix_bath_eval branch October 24, 2022 08:21
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.

Pipeline eval breaks when node has empty answers list in Tutorial 15 TableQA
3 participants