Good errors #12
Good errors #12
Conversation
Pull Request Test Coverage Report for Build 66
💛 - Coveralls |
There was a problem hiding this comment.
It is mostly OK, but there are a few concerns worth addressing. Most importantly, we should not be using unwrap() in documentation. Doctests should be relying on the ? operator instead. C-QUESTION-MARK explains how you can do this.
Not quite important yet, but it might also be worth thinking whether we'll have other kinds of errors to consider in the future. Each change in the error type counts as a breaking change, after all.
src/lib.rs
Outdated
| use std::fmt; | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] | ||
| pub struct LengthError; |
There was a problem hiding this comment.
We could make this type more useful by including the lengths of both predictions and ground truth. The implementation of Display could then be improved accordingly.
There was a problem hiding this comment.
Cleaned up, adds the two variables to the struct and to the display.
|
I also switched |
Well, not for libraries, actually. Those should be clear at compile time what kinds of errors can emerge from an operation, and we cannot do that with a |
Reverts LengthError to Box<Error>
|
Got it, reverted 👍 |
src/lib.rs
Outdated
|
|
||
| impl fmt::Display for LengthError { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| write!(f, "The lengths of the predicted and actual datasets must be equal.\nInstead, the predicted length was found to be {} while the ground truth was found to be {}", self.0, self.1) |
There was a problem hiding this comment.
Can you make this one a bit more concise? (stick to lowercase and one short line of text)
src/lib.rs
Outdated
|
|
||
| impl Error for LengthError { | ||
| fn description(&self) -> &str { | ||
| "The lengths of the predicted and actual datasets must be equal." |
There was a problem hiding this comment.
Likewise, this text can also be shortened. Its usefulness is limited anyway.
src/lib.rs
Outdated
| where | ||
| T: Eq, | ||
| { | ||
| 1.0 - categorical_accuracy(pred, actual) | ||
| let cat_acc = categorical_accuracy(pred, actual); |
There was a problem hiding this comment.
Or maybe just:
let cat_acc = categorical_accuracy(pred, actual)?;
Ok(1. - cat_acc)
|
All resolved! |
src/lib.rs
Outdated
|
|
||
| impl Error for LengthError { | ||
| fn description(&self) -> &str { | ||
| "Dataset lengths are do not match" |
|
Fixed that.. Thank you.. What's more concerning is I somehow proofread that and thought it looked okay |
|
Thank you for the feedback and help! Merged, bumped to 0.3.0 and published. |
This implements the use of
Result<f32, LengthError>for all public functionsEffectively, whenever any of the top-level functions are called, a check is done on the arrays, and they may return a
LengthErrorifactual.len()andpred.len()differ.The goal of this is to not panic when receiving different lengths and instead let the user handle it. This is in relation to #7
Doc tests and unit tests were also updated to reflect this—doc tests primarily using
.unwrap(). The error criteria also has unit tests.