bug: fix MRR and MAP calculations #7841
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Amnah199
added
the
ignore-for-release-notes
Pull Request Test Coverage Report for Build 9467114407Details
💛 - Coveralls |
Amnah199
removed
ignore-for-release-notes
Pull Request Test Coverage Report for Build 9468451282Warning: 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
💛 - Coveralls |
masci
reviewed
Jun 12, 2024
|
thanks for taking care! I just doubled checked some examples with the result from v1 and it looks fine :) |
Pull Request Test Coverage Report for Build 9515678439Warning: 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
💛 - Coveralls |
masci
previously requested changes
Jun 14, 2024
Pull Request Test Coverage Report for Build 9615764716Warning: 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
💛 - Coveralls |
anakin87
reviewed
Jun 24, 2024
| if ground_document.content in retrieved_document.content: | ||
| score = 1 / (rank + 1) | ||
| break | ||
| ground_truth_content = [doc.content for doc in ground_truth if doc.content is not None] |
There was a problem hiding this comment.
Suggested change
| ground_truth_content = [doc.content for doc in ground_truth if doc.content is not None] | |
| ground_truth_contents = [doc.content for doc in ground_truth if doc.content is not None] |
Comment on lines
72
to
87
| average_precision = 0.0 | ||
| relevant_documents = 0 | ||
|
|
||
| for rank, retrieved_document in enumerate(retrieved): | ||
| if retrieved_document.content is None: | ||
| continue | ||
| ground_truth_content = [doc.content for doc in ground_truth if doc.content is not None] | ||
| for rank, retrieved_document in enumerate(retrieved): | ||
| if retrieved_document.content is None: | ||
| continue | ||
|
|
||
| if ground_document.content in retrieved_document.content: | ||
| relevant_documents += 1 | ||
| average_precision += relevant_documents / (rank + 1) | ||
| if relevant_documents > 0: | ||
| score = average_precision / relevant_documents | ||
| if retrieved_document.content in ground_truth_content: | ||
| relevant_documents += 1 | ||
| average_precision += relevant_documents / (rank + 1) | ||
| if relevant_documents > 0: | ||
| score = average_precision / relevant_documents | ||
| individual_scores.append(score) | ||
|
|
||
| score = sum(individual_scores) / len(retrieved_documents) | ||
| score = sum(individual_scores) / len(ground_truth_documents) |
There was a problem hiding this comment.
Not your fault but I had a hard time understanding the algorithm.
- I think we could benefit from a better choice of variable names.
- Adding a comment in the code with the formula/link from which we drew inspiration could also help
Resources I took inspiration from: #1, #2
Something like this:
average_precision_numerator = 0.0
relevant_documents = 0
ground_truth_contents = [doc.content for doc in ground_truth if doc.content is not None]
for rank, retrieved_document in enumerate(retrieved):
if retrieved_document.content is None:
continue
if retrieved_document.content in ground_truth_contents:
relevant_documents += 1
precision_at_k= relevant_documents / (rank + 1)
average_precision_numerator+=precision_at_k
average_precision=0
if relevant_documents > 0:
average_precision = average_precision_numerator / relevant_documents
individual_scores.append(average_precision)
score = sum(individual_scores) / len(ground_truth_documents)(untested)
WDYT?
There was a problem hiding this comment.
Agreed, I also struggled to understand the function initially. Optimized variables can help.
There was a problem hiding this comment.
Also I think precision_at_k is unnecessary and will be extra storage?
There was a problem hiding this comment.
I agree that it is not necessary, but in my opinion it makes the code more readable.
Feel free to make any changes you think appropriate.
Pull Request Test Coverage Report for Build 9648193977Warning: 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
💛 - Coveralls |
anakin87
approved these changes
Jun 24, 2024
| @@ -10,7 +10,7 @@ | |||
| @component | |||
| class DocumentMAPEvaluator: | |||
| """ | |||
| A Mean Average Precision (MAP) evaluator for documents. | |||
| A Mean Average Precision (MAP) evaluator for documents. For details, please refer to the [resource](https://www.pinecone.io/learn/offline-evaluation/). | |||
There was a problem hiding this comment.
Suggested change
| A Mean Average Precision (MAP) evaluator for documents. For details, please refer to the [resource](https://www.pinecone.io/learn/offline-evaluation/). | |
| A Mean Average Precision (MAP) evaluator for documents. |
I would not put this link in the docstring, but I would put it as a comment at the beginning of the run method code to help those who will be working with the code in the future.
| @@ -10,7 +10,7 @@ | |||
| @component | |||
| class DocumentMRREvaluator: | |||
| """ | |||
| Evaluator that calculates the mean reciprocal rank of the retrieved documents. | |||
| Evaluator that calculates the mean reciprocal rank of the retrieved documents. For details, please refer to the [resource](https://www.pinecone.io/learn/offline-evaluation/). | |||
There was a problem hiding this comment.
Suggested change
| Evaluator that calculates the mean reciprocal rank of the retrieved documents. For details, please refer to the [resource](https://www.pinecone.io/learn/offline-evaluation/). | |
| Evaluator that calculates the mean reciprocal rank of the retrieved documents. |
Same as above
Pull Request Test Coverage Report for Build 9659678165Warning: 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
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9659996000Warning: 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
💛 - Coveralls |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related Issues
Proposed Changes:
Fixed errors of MRREvaluator:
Fixed errors of MAPEvaluator:
Fixed errors in pytest file of
test_document_map.py:test_run_with_complex_data()How did you test it?
Ran unit tests for both evaluators
Notes for the reviewer
Verify the correctness of the calculated scores for an example.
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.