Track multi-confidence filtering/displaying by BryonLewis · Pull Request #460 · Kitware/dive

BryonLewis · 2020-11-25T14:45:10Z

Fixes #423

Old style didn't properly support multiple types on a single track and swapping between them based on the selected types and the confidence filter adjustments.

Added a local variable currentConfidencePair to indicate the index of the sorted confidence pairs that should be displayed.
Within useTrackFilters I modified the filtering function to set the currentConfidencePair when it is visible and above the confidenceThreshold. Let me know how you feel about having a side effect inside of .some.
Returned the confidencePairs[currentConfidencePair] when using track.getType() so that it will render the proper type when displayed in AnnotationLayers, and trackList.
Updated useEventChart and useLineChart to utilize the track.getType() function instead of the confidencePairs[0][0] so they would reflect the proper type as well.
Added some tests to the track.spec.ts to test setting the currentConfidencePair, as well as changing the current type.

Old Version:

New Version:

Questions :
(made a determination that these questions will be placed in a new PR or as new issues, I just want to fix the fundamental problem first in a smaller PR. Feel free to add comments)

Determine what we want to do when deleting tracks by type? Do we delete the type from the track, or do we delete the whole track?
Should we display all types as text in the upper right corner? In the older version of VIAME-Web all types were displayed as a list in the upper right corner of the annotation. This can be done with some minor modifications.

subdavis · 2020-11-27T15:25:38Z

Determine what we want to do when deleting tracks by type? Do we delete the type from the track, or do we delete the whole track?

Delete type until only 1 remains, then delete track (IMO).

Should we display all types as text in the upper right corner? In the older version of VIAME-Web all types were displayed as a list in the upper right corner of the annotation. This can be done with some minor modifications.

Yeah I think we should. It would be nice to stack them instead of side-by-side, I think.

subdavis

I had to go pause and have a plate of cold thanksgiving while I thought about this.

the problem

The problem is communicating a new bit of state to consumers of the filteredTracks computed prop. Places that use this prop need to know something new: "What is the confidence pair in this track that caused it be included in this list". Before, that was implicit. There was only 1 possibility. Now there are several.

You've implemented this by sort of stashing this information inside the track object.

I object to that for a couple reasons:

It improperly decouples filteredTracks from the currentConfidencePair. You don't care about currentConfidnecePair as context unless you already have a dependency on filteredTracks.
It adds new state/methods to the track API that aren't really related to a track itself.
Side effects.

Alternative.

It seems to me like the best outcome would be to change the shape of filteredTracks to include this new bit of information.

const filteredTracks: Ref<{
  track: Track,
  context: {
    confidencePairIndex: number;
  };
}[]>

This involves either changing the interface in a bunch of places or having another computed property that strips out just the track part for interfaces where the context isn't needed. IMO it would be best to just change the interface everywhere.

Naturally the enabledTracks interface will change also. I think that's fine. This new context is nice because any process that needs to explain why it filtered something can just tack on new state, and modules that accept a filtered list can specify which explanations they need.

In the short term, if you want to just declare this interface FilterItem {} at the top of useTrackFilters and import it from everywhere, that's perfectly fine with me.

If you would like me to do the tedious chore of changing the interface everywhere, I am happy to do so.

You could also "re-derive" this information in-place whereever you need it. If you have a track, and you have the list of enabledTypes, you can easily figure out which of a track's types caused its inclusion. However, this does involve subtly duplicating some logic which could lead to bugs. I don't recommend this option.

Another thought: we could actually formalize a "context" object for tracks that our various filtering processes could use to stash information like this. The trouble with that is I don't know where the scope of that context ends. Should we actually be keeping a list of IDs for the checkboxes, or should there just be a "checked" context var? Should we have a "selectedTrackId" state, or a selected context var?

We could make sure that the context object was side-effect safe. I don't know if that would be better than the above proposal.

subdavis · 2020-11-27T15:38:15Z

  setType(trackType: string, confidenceVal = 1) {
    const old = this.confidencePairs;
    this.confidencePairs = [[trackType, confidenceVal]];
+    this.currentConfidencePair = 0;


I believe this is out-of-scope for the concerns of track.ts. This is display/visualization state. It would be like adding an isSelected or isChecked property. This causes the UI's display specification to leak into the track API.

subdavis · 2020-11-27T15:40:16Z

-        ));
+        .some(([confkey, confval], index) => {
+          if (confval >= confidenceThresh && checkedSet.has(confkey)) {
+            track.setCurrentConfidencePair(index);


Side effects inside computed functions should really be avoided.

This is especially dicey because parts of the track state are reactive, parts aren't and this could easily cause an infinite loop, so the implementation details of this function are critical to whether or not this code works.

subdavis · 2020-11-27T21:36:49Z

Thought about it some more today.

If you want to do something easier like a dedicated track context, what about this?

class Track {
  /* Disclaimer about usage and reactivity */
  context: Map<string, string | number>;
}

I don't think methods to get/set the map are necessary. just track.context.get('key') and track.context.set('key', 'value')

The map sort of guarantees it's not reactive, and the lack of methods guarantees we aren't bumping the notifier.

…ME-Web into client/confidence-sort

BryonLewis · 2020-12-17T18:31:38Z

Getting back into this I want to attempt to accurately summarize this and the possible implications.

Summarize the highlights:

useTracks has been changed to useFilteredTracks and has a structure indicating the confidencePairIndex which is the current confidence pair that should be visible based on the confidence filter and the visibility of the types.
To accommodate this the track.getType() function now takes in an argument being the index. This allows the current ConfidencePair index to be shown based on the useFilteredTracks.
In LayerManager the dataTrack item has been modified to provide the trackType which is the current type of the track provided by useFilteredTracks, and confidencePairs has been changed to represent the list of confidencePairs, I think it did that originally but at some point it was swapped to just the first confidencePair in the list. I think these variable names make more sense. You have the full list of confidencePairs and the currentPair provided by trackType.
The change to FrameDataTrack required updating the rendering code in each of the layers. This is where it could impact subscribers of vue-media-annotator depending on how they are using this
The TextLayer has been modified with the capability to draw multiple confidence pairs with the current type being closes to the bounding box and having an indicator. Right now I've disabled drawing additional confidencePairs with the text layer by setting additionalNum=0. I believe we may want the user to be able to specify this because some tracks could have 100+ pairs assoicated with them.
There are now helper functions when deleting visible types which will remove confidence pairs from the track. This will also be used when we implement the confidence pair stuff in the TrackDetailsEditor (The old Attributes view for a track).

subdavis

I can't figure out how to get the viewer to show more than 1 confidence pair. Is there a setting somewher? Enabling / disabling types works as expected (the next highest type appears), but I only ever see one even though the code for multiple is clearly here.

subdavis · 2021-02-22T14:36:27Z

    const intervalTree = useIntervalTree();
    const trackMap = useTrackMap();
-    const tracksRef = useEnabledTracks();
+    const filteredTracksRef = useEnabledTracks();


Please call this tracksRef or more specifically enabledTracksRef. filteredTracks means something else already.

I know this will need to change in a lot of places, but having filteredTracks mean several different things is probably not a good idea.

subdavis · 2021-02-22T14:37:16Z

      editingTrack: false | EditAnnotationTypes,
      selectedTrackId: TrackId | null,
-      tracks: readonly Track[],
+      filteredTracks: readonly FilteredTrack[],


Rename to tracks or enabledTracks.

subdavis · 2021-02-22T14:52:31Z

  y: number;
  offsetY?: number;
  offsetX?: number;
+  currentPair?: boolean;


Please add a comment describing what this is.

Also, maybe make this a required property? You're already providing it everywhere, so the code makes no use of the condition where currentPair: undefined, and it's nice to not have to worry about the undefined check if that's not a valid, meaningful state.

subdavis · 2021-02-22T15:01:03Z

 * @returns value or null.  null indicates that the text should not be displayed.
 */
-function defaultFormatter(track: FrameDataTrack): TextData[] | null {
+function defaultFormatter(track: FrameDataTrack, additionalNum = 0): TextData[] | null {


additionalNum is sort of an awkward way of specifying a max value. Does it mean 1 will always be shown, plus some additionalNum?

Could we just make this maxLines or maxPairs and default it to 1? I think that's clearer, then you wouldn't have to do (maxPairs + 1) below (which is odd anyway because it means that maxPairs = 0 will show 1 line)

I think restructuring this will make it clearer by moving and renaming maxPairs to maxAdditionalPairs down closer to where it is drawn instead of with the default type:confidence pair.

subdavis · 2021-02-22T15:26:40Z

+      let pairCount = 0;
+      if (track.confidencePairs.length) {
+        for (let i = 0; i < track.confidencePairs.length; i += 1) {
+          if (pairCount >= maxPairs) {


If you change maxPairs, this line has to change, but I think it should because this isn't what you'd expect if maxPairs wasn't already off-by-one.

subdavis · 2021-02-22T15:31:57Z

+      if (this.confidencePairs[index]) {
+        return this.confidencePairs[index];
+      }


This doesn't seem right.

if confidencePairs is [['foo', 0.5], ['bar', 0.6]] and I call getType(20000), I'll get ['foo', 0.5]. I think I should get null.

Edit, from above, perhaps this should just raise an out of bounds exception?

I was maintaining parity with the existing getType() which returned the 0 indexed confidencePair if it existed or else it would return null. So I modified it to return the 0 index confidencePair by default if it existed and index was out of scope. Or else it would return null if the confidencePairs were empty.

We can change this but I'll need to review and understand why we had it like this in the first place. I imagine it has something to do with creation or CSV files which don't always contain types.

subdavis · 2021-02-22T15:37:24Z

      if (confidencePairs.length) {
-        const trackType = confidencePairs[0][0];
+        const type = track.getType(filtered.context.confidencePairIndex);
+        const trackType = type ? type[0] : '';


This is causing me to question the signature of track.getType()

It should probably always return [string, number] instead of the possibility of a null type, right? Like, if you ask for an out-of-bounds pair, that seems like an invalid execution path that should also throw an error from within the getter.

Why would getType ever be null? Why would you ever have a reference to a confidencePair index that doesn't exist?

BryonLewis · 2021-02-22T16:11:20Z

I can't figure out how to get the viewer to show more than 1 confidence pair. Is there a setting somewher? Enabling / disabling types works as expected (the next highest type appears), but I only ever see one even though the code for multiple is clearly here.

Here I set additionalNum to 2. The current value is closest to the box and more opaque than the additional values added. It also has the ** next to it which can be removed or changed.

It should be the default value of additionalNum in the defaultFormatter function. By default we always want to display the highest confidence pair for the text value. I don't know the exact format we want for additional values to be displayed. Do we want the top X values? If it has 200 values and we are filtered down to item number 50, do the top 5 values show any relevance? Should we take the closes X values in distance to the current value. Meaning do we grab x/2 above and below the current value and only below if this is the top value already? I left the display code in there as a beginning for supporting more visually. I y believe this needs to be decided before we activate it as well as allowing users to specify themselves how many additional pairs they see.

Co-authored-by: Brandon Davis <brandon.davis@kitware.com>

BryonLewis · 2021-02-23T18:36:38Z

@subdavis hopefully I didn't botched this up too horribly. I believe that I addressed all of your comments and tried to make things clearer. After looking at what calls getType there seems to be no reason for it to not through an error if the confidencePairs[index] doesn't exist. We will just need to ensure in the future on import that all tracks have a default pair (unknown, 1.0) if nothing exists.

subdavis · 2021-02-25T15:59:45Z

Pending your thoughts on #603, this is good to go.

* defaultFormatter * simpler syntax * comments * Change line height

subdavis

Thanks for making these changes. I think just changing the names for enabledTracks and TrackWithContext are the biggest improvements.

BryonLewis and others added 2 commits November 25, 2020 09:29

Fixing confidence sorting

fad5bc0

Merge branch 'master' into client/confidence-sort

978d972

BryonLewis marked this pull request as ready for review November 25, 2020 18:54

subdavis suggested changes Nov 27, 2020

View reviewed changes

BryonLewis and others added 10 commits November 30, 2020 11:18

Switched to useFilteredTracks

708c50d

Merge branch 'client/confidence-sort' of https://github.com/VIAME/VIA…

7e16047

…ME-Web into client/confidence-sort

Merge branch 'master' into client/confidence-sort

d8a5ca4

Merge branch 'master' into client/confidence-sort

33d01ab

Modify trackdata for types

e95e44d

Adding in text display and filtering

e445567

Merge branch 'master' into client/confidence-sort

e1e091b

updating for the defaultFormatter to work

681b33b

mend

9945643

Merge branch 'master' into client/confidence-sort

109eebc

BryonLewis mentioned this pull request Feb 1, 2021

[BUG] [Client] Type visibility controls seem broken #552

Closed

BryonLewis and others added 3 commits February 1, 2021 10:52

Merge branch 'master' into client/confidence-sort

4226451

Merge branch 'master' into client/confidence-sort

9421cf4

Updating text displaying

58d1611

BryonLewis linked an issue Feb 11, 2021 that may be closed by this pull request

[BUG] [Client] Type visibility controls seem broken #552

Closed

fix unused

2438626

subdavis self-requested a review February 18, 2021 19:59

subdavis suggested changes Feb 22, 2021

View reviewed changes

BryonLewis and others added 4 commits February 22, 2021 11:29

Update client/src/use/useTrackFilters.ts

2662a79

Co-authored-by: Brandon Davis <brandon.davis@kitware.com>

Update client/src/use/useLineChart.ts

297e51a

Co-authored-by: Brandon Davis <brandon.davis@kitware.com>

Update client/src/use/useTrackFilters.ts

c8c6bea

Co-authored-by: Brandon Davis <brandon.davis@kitware.com>

addressing comments

392e312

BryonLewis added 2 commits February 23, 2021 13:02

converting getType to remove null return type

1fdd2f3

some final fixes

e6402d3

BryonLewis requested a review from subdavis February 23, 2021 18:53

Confidence sort brandon (#603)

1fccd2f

* defaultFormatter * simpler syntax * comments * Change line height

subdavis approved these changes Feb 25, 2021

View reviewed changes

BryonLewis merged commit 0035e19 into master Feb 25, 2021

BryonLewis deleted the client/confidence-sort branch February 25, 2021 16:49

subdavis restored the client/confidence-sort branch February 26, 2021 01:25

subdavis mentioned this pull request Feb 26, 2021

Client/confidence sort #605

Merged

Conversation

BryonLewis commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

subdavis commented Nov 27, 2020

Uh oh!

subdavis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

the problem

Alternative.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subdavis commented Nov 27, 2020

Uh oh!

BryonLewis commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

subdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subdavis Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryonLewis commented Feb 22, 2021

Uh oh!

BryonLewis commented Feb 23, 2021

Uh oh!

subdavis commented Feb 25, 2021

Uh oh!

subdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BryonLewis commented Nov 25, 2020 •

edited

Loading

subdavis left a comment •

edited

Loading

BryonLewis commented Dec 17, 2020 •

edited

Loading

subdavis Feb 22, 2021 •

edited

Loading