-
Notifications
You must be signed in to change notification settings - Fork 449
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 recall #2518
base: main
Are you sure you want to change the base?
Add recall #2518
Conversation
…h RecallMetric. Adapt confusionStats and Precision with generalized code.
… rename test function properly in precision.
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 have a couple of comments, otherwise good start! 🙂
pub enum ClassificationConfig { | ||
Binary { | ||
threshold: f64, | ||
class_reduction: ClassReduction, |
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 I would leave the class reduction out of the config. It is mandatory for all classification types based on the metric anyway, and it's weird that the same field is repeated.
} | ||
} | ||
|
||
pub enum ClassificationConfig { |
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 name might be a little too generic. I suggest ClassificationMetricConfig
.
|
||
/// Input for classification metrics | ||
#[derive(new, Debug, Clone)] | ||
pub struct ClassificationInput<B: Backend> { |
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.
This is a bit too broad since we have other classification metrics already. Since this is relevant to the confusion statistics computed, I suggest that it is renamed to something like ConfusionStatsInput
.
Lmk what you think.
/// Sets the class reduction method. | ||
#[allow(dead_code)] | ||
pub fn with_class_reduction(mut self, class_reduction: ClassReduction) -> Self { | ||
self.class_reduction = class_reduction; | ||
self | ||
} | ||
|
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.
With the suggested change to remove class reduction from the config, I would keep this.
class_reduction: ClassReduction, | ||
config: PrecisionConfig, | ||
config: ClassificationConfig, |
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 forgot the separate class_reduction
field. But anyway with the suggested change it would remain 🙂
Pull Request Template
Checklist
run-checks all
script has been executed.Related Issues/PRs
#544
Changes
Create
ClassificationInput
andClassificationConfig
, generalizations ofPrecisionInput
andPrecisionConfig
for any classification task (currently used by precision and recall).Add
RecallMetric
.Moved ClassReduction into ClassificationConfig, since i think they belong together.
Testing
Added tests for recall