RSDK-2380 Check if labels file is split by spaces or commas #2178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viambot
added
the
safe to test
kim-mishra
commented
Apr 6, 2023
| classifications, err := unpackClassificationTensor(ctx, outTensor, model, labels) | ||
| if err != nil { | ||
| logger.Error(err) |
There was a problem hiding this comment.
What is the best way to surface this error to the user? Logging it here logs it over and over and over again, but just returning the error doesn't seem to surface it
There was a problem hiding this comment.
Yeah this function that gets returned will run every time an image gets classified. My first thought is that if we hate that, maybe try a logger.Fatal() which will kill everything after printing it out once. If that seems like overkill, maybe only fatal depending on the type of error. Either way it seems like don't want to keep on trying to classify this over and over again if we're definitely not unpacking the tensors correctly.
There was a problem hiding this comment.
hmm the linter complains about logger.Fatal, do you know of a workaround?
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
…ls do not match outputs
viambot
added
safe to test
viambot
added
safe to test
bhaney
requested changes
Apr 7, 2023
Comment on lines
420
to
427
| // if the labels come out as one line, try splitting that line by spaces or commas to extract labels | ||
| if len(labels) == 1 { | ||
| labels = strings.Split(labels[0], " ") | ||
| } | ||
|
|
||
| if len(labels) == 1 { | ||
| labels = strings.Split(labels[0], ",") | ||
| } |
There was a problem hiding this comment.
let's try a comma-split first, before the space-split. I'm thinking about the case where someone has the label "stop sign", and it gets split on line 422, before 426 is attempted. Also, I think people often put spaces after their commas, so the space-split may grab commas and put them in the label string that would show up in the overlay.
| @@ -254,3 +255,23 @@ func TestMoreClassifierModels(t *testing.T) { | |||
| test.That(t, bestClass[0].Label(), test.ShouldResemble, "292") | |||
| test.That(t, bestClass[0].Score(), test.ShouldBeGreaterThan, 0.93) | |||
| } | |||
|
|
|||
| func TestInvalidLabels(t *testing.T) { | |||
There was a problem hiding this comment.
Could you also add a test of it successfully unpacking a label file where all the labels are on one string, comma separated?
…d texgt & necessary files
viambot
added
safe to test
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.