Skip to content
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

Quiver timeline - add filters #62

Closed
wants to merge 6 commits into from

Conversation

noornoorie
Copy link
Contributor

Closes #45

@noornoorie noornoorie marked this pull request as ready for review February 6, 2024 15:40
@paulpestov
Copy link
Contributor

First of all, good job on that one! I have some comments:

I think the processor filters should also filter whole workflows because we want to see workflows that contain a certain processor and hide workflows that do not contain the selected processor(s). Also it should be a multiselect filter switching that filter to inspect 2 or more processors may be tidious.

@noornoorie
Copy link
Contributor Author

I've changed processor dropdown to multiselect as you requested. However, I'm not sure about the other requirement. Should the two dropdowns be mutually connected so that changing one would change the values of the other? Or should it work the opposite of what's currently implemented - selecting processor should change the values of workflow dropdown?

@paulpestov
Copy link
Contributor

paulpestov commented Feb 27, 2024

... However, I'm not sure about the other requirement. Should the two dropdowns be mutually connected so that changing one would change the values of the other? Or should it work the opposite of what's currently implemented - selecting processor should change the values of workflow dropdown?

I think there is a misunderstanding what the filter should do. I think it should be like "show only GT items that contain selected processors". Not just show the selected processors and hide not selected ones, this gives no benefit, it just hides some array items.

I see now also the "workflow filter" should behave the same.

So each of the filters above the GT items list should filter out GT items to enable users to find better their desired GT item. So, no, there is no connection between the filters themselves (yet). It is basically an AND conjuction of selected filter values.

@noornoorie
Copy link
Contributor Author

I believe I'm already filtering the GT items here?

const gtList = computed<GroundTruth[]>(() => {
  return workflowsStore.gt.filter(({ id, metadata }) => {
    let flag = filtersStore.gt.findIndex(({ value }) => value === id) > -1
    if (flag && selectedDateRange.value) {
      const splitDateRange = selectedDateRange.value.value.split('-')
      const from = splitDateRange[0]
      const to = splitDateRange[1]

      flag = flag && (metadata.time.notBefore === from && metadata.time.notAfter === to)
    }
    if (flag && selectedWorkflow.value) {
      flag = flag && workflowsStore.getRuns(id, selectedWorkflow.value.value).length > 0
    }
    if (flag && selectedWorkflowSteps.value.length > 0) {
      selectedWorkflowSteps.value.forEach(({ value }) => {
        if (!flag) {
          return
        }
        flag = flag && workflowsStore.getRuns(id).findIndex(({ metadata }) => {
          return metadata.workflow_steps.findIndex(({ id }) => id === value) > -1
        }) > -1
      })
    }
    return flag
  })
})

@paulpestov
Copy link
Contributor

This is what happens when we select something from the processor filter. It seems to filter out the display in the workflows table. I realize that the issue is that we might never even able to filter any GT items because of course we have each processor in all 4 workflows in all GT items. So this filter might be quite useless at this point and only start working when we apply some different workflows on different GT items. Maybe @MareenGeestmann can say somethign about this.
processir-filter

@MareenGeestmann
Copy link

As for now all GT documents were run with all displayed workflows filtering by processors will not reduce the list of documents. Therefore, we do not need to implement it (for now).

@noornoorie
Copy link
Contributor Author

@MareenGeestmann So should I just remove the GT list filtering part - keeping the dropdowns to control the visibility of workflows and processors inside the GT items, or should I drop the pull request altogether?

@MareenGeestmann
Copy link

@MareenGeestmann So should I just remove the GT list filtering part - keeping the dropdowns to control the visibility of workflows and processors inside the GT items, or should I drop the pull request altogether?

Let us discuss this with @paulpestov.

@paulpestov
Copy link
Contributor

paulpestov commented Mar 19, 2024

I think we should keep the processor filter. We know that right now every GT item is executed with every workflow (and therefore every processor is present) but that might change in the future. I think the combination which GT is executed with which workflow is just a configuration thing of the Quiver backend and not a feature per se. So it CAN be changed very easily. For that case we will already have a good filter.

@MareenGeestmann So should I just remove the GT list filtering part - keeping the dropdowns to control the visibility of workflows and processors inside the GT items, ...

No, it should be the exact opposite: remove the control of visibility of workflows and processors inside the GT items and add the GT list filtering part.

As mentioned before, all filters should be about filtering GT items. Nothing should change within a GT item. Though, if we wish so, we should open a new issue for filtering information inside a GT item.

@paulpestov
Copy link
Contributor

One more thing: could you set all checkboxes to checked at the processor filter? Right now they are initially all unchecked which is kinda false since we see "all GT items with all processors" at the first load.

@noornoorie
Copy link
Contributor Author

@paulpestov i've coded such that the behavior is the same with all the processors checked and with all unchecked. Pre-selecting all the processors makes filtering them harder, as noted in issue #64. Do you have any advice regarding this issue?

@paulpestov
Copy link
Contributor

I still think what I wrote in the comments above should be the correct behaviour. The search is just a little add-on and could be omitted.

@noornoorie noornoorie force-pushed the ticket-45-filtering branch from 95cf1a8 to 3387ceb Compare April 17, 2024 15:04
@noornoorie
Copy link
Contributor Author

@paulpestov I've added the checkboxes on page load as requested

@paulpestov
Copy link
Contributor

Thanks, but what about the filtering GT stuff? That:

I think we should keep the processor filter. We know that right now every GT item is executed with every workflow (and therefore every processor is present) but that might change in the future. I think the combination which GT is executed with which workflow is just a configuration thing of the Quiver backend and not a feature per se. So it CAN be changed very easily. For that case we will already have a good filter.

@MareenGeestmann So should I just remove the GT list filtering part - keeping the dropdowns to control the visibility of workflows and processors inside the GT items, ...

No, it should be the exact opposite: remove the control of visibility of workflows and processors inside the GT items and add the GT list filtering part.

As mentioned before, all filters should be about filtering GT items. Nothing should change within a GT item. Though, if we wish so, we should open a new issue for filtering information inside a GT item.

@MareenGeestmann
Copy link

Open:
Hide row where workflow contains not the filtered processor. (keep all processors visible)
Hide whole GT item when processor does not exist in all workflows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quiver timeline - add filters
3 participants