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

Add docu of confidence scores and calibration method #1131

Merged
merged 5 commits into from
Jun 3, 2021

Conversation

julian-risch
Copy link
Member

@julian-risch julian-risch commented Jun 2, 2021

Proposed changes:

  • Add more detailed documentation of confidence calibration to the reader's usage page.
  • Add calibrate_confidence_scores() method to reader, which internally calls eval() method

closes #1032

@julian-risch julian-risch requested a review from tholor June 2, 2021 10:04
Copy link
Member

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

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.
Copy link
Member

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,
Copy link
Member

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 🤔

Copy link
Member Author

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

@julian-risch julian-risch merged commit 580e283 into master Jun 3, 2021
@julian-risch julian-risch deleted the confidence-documentation branch June 3, 2021 13:49
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.

Simplify usage of confidence scores & add documentation
2 participants