-
Notifications
You must be signed in to change notification settings - Fork 129
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
Threshold selection tool #2003
Threshold selection tool #2003
Conversation
Thanks both. I agree that from a purely information perspective the sigmoid doesn't add anything, but I think it helps make the information more accessible by facilitating different approaches to it. Whether you want to wiggle your cursor up and down the match probability axis or left and right across the match weight axis, it works equally well, while also literally drawing your attention to how your preferred measure maps onto the other. For example:
I've never liked that axis on the waterfall chart, but that's a rare example of something that only works in a match weight context, so the funky axis is the only way to visually cater for match probability. |
Is "score" a match probability? If yes, then it seems possible to optionally add back the sigmoid but still only use two charts (that is the confusion matrix plus one combined chart)!? In the "Score" chart, the y-axis begins at 0.5. How does it look if it begins at 0? For titles, I suggest the y-axis title "Match probability (Score)" and the x-axis title "Match weight". Then, how does it look like to add (overlay) the sigmoid curve on this chart? |
I think the y axis 'score' refers to the 'precision or recall score'. (just 'score' because it's used for both metrics). But can see that's a potential source of confusion and perhaps we could find a better name ('performance metric score' perhaps?) |
The name 'performance metric score' is more general than 'precision or recall score', so better in that sense. My suggestion is to overlay (superimpose) the 'sigmoid' so that the result is one combined chart with two chart areas, not three chart areas. This way, we do require another chart area for the sigmoid. |
I experimented with being able to zoom in on the y ("Score") axis because the [0,1] range was too big to be useful, but I gave up because it was too difficult to constrain it the way I wanted (with 1 always the maximum). The reason for the compromise shown here is that if precision/recall/F1 are below 0.5 your model isn't worth pursuing with. The area of interest is loosely described as "close to 1" in the score axis. In the example shown, the ideal range would be something like [0.85,1], and a [0,1] would contain a lot of whitespace and make it difficult to see the detail where needed. Here's an example showing a less ambiguous y-axis title and with the full [0,1] range: |
42349bd
to
946917a
Compare
Well, I think this latest combined chart is easier to interpret thanks to the full [0,1] range in the y-axis. For completeness, could you add back the sigmoid chart? That is, I would like to see the first chart but with modification of the full [0,1] range in the y-axis as in the latest chart. |
c57830b
to
d123930
Compare
Should we remove Given the functionality is so similar I wouldn't be worried about breaking backwards compatibility |
Yes, I was thinking about that in #2010 |
@samnlindsay i edited the above because you referenced 2020, but i think you probably meant 2010 |
Yes sorry I was on my phone |
No worries, just didn't want to edit without you knowing in case i did it wrong! |
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.
The new code looks good to me.
Please can we remove linker.confusion_matrix_from_labels_table
and associated code/docs as part of the PR
Co-authored-by: Robin Linacre <robin.linacre@digital.justice.gov.uk>
Co-authored-by: Robin Linacre <robin.linacre@digital.justice.gov.uk>
The one exception is the blog post announcing the confusion matrix chart as a new feature. The image shown is unchanged but it now links to the new threshold selection tool notebook instead of the confusion matrix one. |
linker.confusion_matrix_from_labels_table("labels") | ||
``` | ||
```py | ||
linker.accuracy_chart_from_labels_column("ground_truth", add_metrics=["f1"]) |
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.
I think this may need to be changed to linker.threshold_selection_tool_from_labels_table(...)
I definitely would like to see the confusion matrix by default. It does not help to hover over one of the graphs when the output is PDF :-) |
Type of PR
Is your Pull Request linked to an existing Issue or Pull Request?
Relevant to #1564 in that it ties together several different charts into a single tool.
Creating draft PR while I gather feedback and then I'll add docs etc.
Do we want to replace the individual charts that this combines? Or add this as another function? Only it duplicates a lot of code and it's harder to maintain all of them, especially if they're not being used.
@ThomasHepworth FYI
Give a brief description for the solution you have provided
Combined interactive chart to:
Screenshot (using pairwise_labels.ipynb)
PR Checklist