-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add docu of confidence scores and calibration method #1131
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.
Left two minor comments.
Ready to merge once the documentation one is resolved. With the other comment I would rather like to understand if that's worth opening a new issue and small extra PR.
docs/_src/usage/usage/reader.md
Outdated
Note that a finetuned confidence score is specific to the domain that its finetuned on. | ||
on a specific dataset. | ||
To this end, the reader has a `calibrate_confidence_scores()` method. | ||
This method needs the has the same input parameters as the `eval()` method because the calibration of confidence scores is performed on a dataset that comes with gold labels. |
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.
Typo: "needs the has the.."
"has the same input parameters as the eval()
method" => Let's avoid that users start here to search for the eval method to understand what params this one expects. Let's rather put all information needed directly here or linking the API reference for calibrate_confidence_scores(). I guess you are already trying to say that documentstore is the only major parameter needed here, but I wouldn't really understand it that way by just reading the text.
def calibrate_confidence_scores( | ||
self, | ||
document_store: BaseDocumentStore, | ||
device: str, |
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.
Do we actually need the device
here as mandatory ar or better so say: do we need it in eval()? The reader should already be placed on GPU or CPU from the init. We might only need device
if we want to change it 🤔
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.
You are right. I added an issue #1137
Proposed changes:
calibrate_confidence_scores()
method to reader, which internally callseval()
methodcloses #1032