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

feat: Change outputs of AnswerExactMatchEvaluator #7390

Merged
merged 4 commits into from
Mar 26, 2024

Conversation

silvanocerza
Copy link
Contributor

Proposed Changes:

Standardize outputs with other evaluators.
This PR also adds a new test with more complex data.

How did you test it?

I ran tests locally.

Notes for the reviewer

This decision stems from this discussion.

I ignored the release notes as this has still not been released and the PR that added this Component already has release notes.

Checklist

@silvanocerza silvanocerza added the ignore-for-release-notes PRs with this flag won't be included in the release notes. label Mar 20, 2024
@silvanocerza silvanocerza self-assigned this Mar 20, 2024
@silvanocerza silvanocerza requested a review from a team as a code owner March 20, 2024 16:28
@silvanocerza silvanocerza requested review from shadeMe and julian-risch and removed request for a team March 20, 2024 16:28
@github-actions github-actions bot added topic:tests 2.x Related to Haystack v2.0 type:documentation Improvements on the docs labels Mar 20, 2024
@silvanocerza silvanocerza requested a review from shadeMe March 20, 2024 16:59
@coveralls
Copy link
Collaborator

coveralls commented Mar 20, 2024

Pull Request Test Coverage Report for Build 8377370259

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 34 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.02%) to 89.282%

Files with Coverage Reduction New Missed Lines %
components/generators/chat/hugging_face_tgi.py 5 94.79%
components/generators/hugging_face_local.py 6 91.89%
core/pipeline/pipeline.py 23 93.7%
Totals Coverage Status
Change from base Build 8362383368: 0.02%
Covered Lines: 5423
Relevant Lines: 6074

💛 - Coveralls

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

I disagree with Changes scores to return the number of matches per question. It differs from the semantics we had in the past for ExactMatch in Haystack and for the use cases we so far support.
If there are multiple answers predicted for one query, they are top k answer candidates in a specific order and we don't aim to cover multiple ground truth questions with them.
Multiple ground truth answers can come from annotations with slightly different punctuation etc. For the users, there is no additional value if the predicted answers for one query contain more than one ground truth answer. Let's leave the metric as is.

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@silvanocerza silvanocerza merged commit f398b29 into main Mar 26, 2024
20 checks passed
@silvanocerza silvanocerza deleted the exact-match-change-output branch March 26, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Related to Haystack v2.0 ignore-for-release-notes PRs with this flag won't be included in the release notes. topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants