Fix confusion matrix using only predictions as source for labels #249
Conversation
Add serialization for LogisticRegression
Serialization for multi-class
Float type restriction with handwritten bounds
Confusion matrix should use labels from predictions and ground truth
Codecov ReportBase: 39.24% // Head: 39.26% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #249 +/- ##
==========================================
+ Coverage 39.24% 39.26% +0.02%
==========================================
Files 92 92
Lines 6085 6089 +4
==========================================
+ Hits 2388 2391 +3
- Misses 3697 3698 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
This is the correct test
|
The argument is |
| @@ -323,6 +323,18 @@ pub trait Labels { | |||
| fn labels(&self) -> Vec<Self::Elem> { | |||
| self.label_set().into_iter().flatten().collect() | |||
There was a problem hiding this comment.
For some reason this method doesn't dedup the final vector. It should do something like union all HashSet together. Or we can just change the return type to HashSet, but that might be too invasive.
| @@ -323,6 +323,18 @@ pub trait Labels { | |||
| fn labels(&self) -> Vec<Self::Elem> { | |||
| self.label_set().into_iter().flatten().collect() | |||
| } | |||
|
|
|||
| fn combined_labels(&self, other: Vec<Self::Elem>) -> Vec<Self::Elem> { | |||
There was a problem hiding this comment.
Better to have this method take &impl Labels or &Self as input. Then you can call label_set on both self and the input and union all the hashsets before converting it into a Vec.
Fix confusing matrix incorrectly using labels from predict only instead of using labels from predict and ground truth. Ideally we should expose the Scikit-like API that passes in all the labels, in case the labels in the test set are not all inclusive (which would be a mistake in train/test partitioning, but can happen).
I'm somewhat confused by the way the API is written because the argument for the
confusion_matrixmethod is calledground_truth, but shouldn't it be the predicted points instead?