Confidence sort brandon#603
Merged
Merged
Conversation
BryonLewis
requested changes
Feb 25, 2021
Collaborator
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
I like it simplified but I think we need to account for indices outside of the max pairs number.
| * @returns value or null. null indicates that the text should not be displayed. | ||
| */ | ||
| function defaultFormatter(track: FrameDataTrack, additionalNum = 0): TextData[] | null { | ||
| function defaultFormatter(track: FrameDataTrack, maxPairs = 3, lineHeight = 20): TextData[] | null { |
Collaborator
There was a problem hiding this comment.
Only thing is if we want to have maxPairs = 3 right now given how cluttered it could look for datasets with a lot of detections on screen at once?
Contributor
Author
There was a problem hiding this comment.
Oh sure, I can reduce that to 1.
| currentHeight += lineHeight; | ||
| pairCount += 1; | ||
| } | ||
| const totalVisiblePairs = Math.min(track.confidencePairs.length, maxPairs); |
Collaborator
There was a problem hiding this comment.
What if my trackType is outside of the maxPairs number? We don't re-sort the pairs list when changing filter or altering types we change the index into the currentPairs list for the getType. So if max pairs was 3 and my currentPairIndex was 9 you wouldn't have the proper name associated with the current type.
maxPairs.mp4
BryonLewis
added a commit
that referenced
this pull request
Feb 25, 2021
* Fixing confidence sorting * Switched to useFilteredTracks * Modify trackdata for types * Adding in text display and filtering * updating for the defaultFormatter to work * mend * Updating text displaying * fix unused * Update client/src/use/useTrackFilters.ts Co-authored-by: Brandon Davis <brandon.davis@kitware.com> * Update client/src/use/useLineChart.ts Co-authored-by: Brandon Davis <brandon.davis@kitware.com> * Update client/src/use/useTrackFilters.ts Co-authored-by: Brandon Davis <brandon.davis@kitware.com> * addressing comments * converting getType to remove null return type * some final fixes * Confidence sort brandon (#603) * defaultFormatter * simpler syntax * comments * Change line height Co-authored-by: Brandon Davis <brandon.davis@kitware.com>
subdavis
added a commit
that referenced
this pull request
Mar 1, 2021
* Fixing confidence sorting * Switched to useFilteredTracks * Modify trackdata for types * Adding in text display and filtering * updating for the defaultFormatter to work * mend * Updating text displaying * fix unused * Update client/src/use/useTrackFilters.ts Co-authored-by: Brandon Davis <brandon.davis@kitware.com> * Update client/src/use/useLineChart.ts Co-authored-by: Brandon Davis <brandon.davis@kitware.com> * Update client/src/use/useTrackFilters.ts Co-authored-by: Brandon Davis <brandon.davis@kitware.com> * addressing comments * converting getType to remove null return type * some final fixes * Confidence sort brandon (#603) * defaultFormatter * simpler syntax * comments * Change line height Co-authored-by: Bryon Lews <Bryon.Lewis@kitware.com> Co-authored-by: BryonLewis <61746913+BryonLewis@users.noreply.github.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
There was still an off-by-1 issue with this function. If you provided
additionalNum>confidencePairs.length - 1you'd end up with a height miscalculation for lines n>1However, I think the function was generally more complicated than it needed to be. Here's a version with a simple
maxLinesarg that will show up tomaxLinesrows. You don't have to think about any off-by-one conditions because there's just a single loop and no special external conditions or break condition.