Adds feature for filtering annotations by shapes and user in Largo #193
Conversation
…e, and integrate with the component. The component is still not working as expected, however API routes are working.
This feature should be improved by making it load shapes from the DB on load rather than on click. Temporary checkpoint because of change of pc.
This commit implements the filtering functionality for the tab, which now fetches only the images required. For now the only filter implemented is the `shape` filter, but this implementation should allow for an easy expansion with any other filter.
This reverts commit fd29dd1. This commit inadvertently introduces an autoformatting that makes the diffs unreadable.
Readds filtering functionality for shapes, that allows for expanding with other filters.
This commit implements the possibility of filtering images by users. Implements the select button as well as the js components required.
…unctions In this commit, code that was previously imcomplete (e.g. API calls, filtering tab loading user names incorrectly) gets fixed by refining functions and missing API calls.
|
Wow. Cool PR. Thanks Davide |
There was a problem hiding this comment.
Awesome, thanks! High-level comments first (in no particular order). I'll look at the code in a later review.
-
The BIIGLE modules track their compiled assets (JS/CSS) as well. So once you have something ready to review/publish, please run
npm run prodonce and commit the minified assets. Otherwise you end up with a 4k lines JS file like now. -
When a filter is active, the sidebar tab must be highlighted. Compare this with the sorting tab when a sorting is active:
This is important so users don't forget about the filters once the tab is closed.
-
There is no need to make an API call to fetch the available annotation shapes. You can hard-code them in the template. Either directly as HTML or you populate a JS variable (with
biigle.$declare()andbiigle.$require()). Also, if Largo is opened for a single volume and it is not a video volume, the WholeFrame shape should not be shown. -
To make the objective speed faster, you can perform the API call to fetch available users when the filter tab is open (instead of when the select input is focussed). Then the response will hopefully be already there when the user selects the input.
-
Ideally, I want the filter tab here to work similar to the filter tab of the volume overview (except the flag/filter switch and the show filenames button):
If you want you can implement it this way here. Otherwise we can keep your UI with the select inputs as first version and then update the UI later.
There were two things that didn't work for me:
-
When I open Largo for a whole project, I get an empty response from the "users-with-annotations" API endpoint.
-
When I click on a label (with annotations) and thenn immediately click on another label (without annotations), it shows the message "no annotations" for both labels. I even see a flash of annotation patches of the first label while it loads. But somehow the caching is messed up afterwards. I don't see this behavior in the version on biigle.de so I assume that it has something to do with your changes. Here is an example:
Screencast.from.04.11.2024.14.42.57.webm
This commit introduces highlighting to the filters tab by using the activeFilters property
…alues of these filters This commit changes fundamentally the way the filters are loaded; in particular, they are loaded not on click, rather on page load. To do so, the filters are loaded and then injected inside the template view, removing the need of further API calls.
This commit introduces a system that allows the selection of multiple values in the filter tab, so that it is now possible to combine multiple filters. This comes with logical operators (is, is not, or, and). Changes the functioning of the API endpoints for filters of users and shapes as well. In a further commit, tests will be introduced.
I am not able to run
This should be done now!
Done for the most part, need to remove the WholeFrame shape. EDIT: I fixed this part
I managed to add this as the page loads, lmk if it seems good to you!
This took almost the whole week, but it seems to work now. Not the prettiest, but I'd like to now if the way it functions now is good for you. It was fun, and I hope the code is not too messy.
Since there is no API anymore, it should not appear now!
I was not able to reproduce this, but then again with all the new changes a lot has been modified and it could be that I inadvertently fixed it while changing something else. Thanks for the feedback! |
Hi @mzur, yes the sorting should be okay now. Actually, I realized that I was approaching the filtering the wrong way, since adding the new data to the |
This commit changes variable names and changes activeFilters to computed value. It also fixes a bug with the selectedFilterValues variable that caused sometimes the filters to behave incorrectly.
There was a problem hiding this comment.
When I go to the "relabel" step, the filter remains active but has no effect. We can either disable the filter tab (as with the sorting) or reset the filter rules and allow the user to enable a new filter that works for the relabel step. In this case, if the user goes back to "dismiss", the filter rules should be reset again, too. What do you think?
Screencast.from.2025-03-06.10-02-49.webm
Also I saw lots of JS lines that weren't termiated with a ;. While this is optional in JS, you should adhere to the coding style of this project and add them. Coding style is important in a larger project with multiple collaborators, so please watch your spaces and semicolons in the future 😉
|
|
||
| let cacheKey = JSON.stringify(filterLabel); | ||
| if (this.hasActiveFilters && this.filtersCache.hasOwnProperty(filtersCacheKey)) { | ||
| annotations = annotations.filter(annotation => this.filtersCache[filtersCacheKey].includes(annotation.id)); |
There was a problem hiding this comment.
This looks a little inefficient, since includes() could traverse the whole array each time. What about adding an object/map instead of an array to filtersCache when it is populated ({id: true})? Then you could do O(1) lookups here (this.filtersCache[filtersCacheKey][annotation.id] === true).
I'm a little sensitive about performance here because you can have hundreds of thousands of annotations in Largo.
| @@ -139,7 +138,7 @@ export default { | |||
| return annotations; | |||
| }, | |||
| hasNoAnnotations() { | |||
| return this.selectedLabel && !this.loading && this.annotations.length === 0; | |||
| return this.selectedLabel && !this.loading && this.sortedAnnotations.length === 0; | |||
There was a problem hiding this comment.
Why this change? Shouldn't the annotations and sorted annotations have equal length?
| labelFilters['filters'] = filters; | ||
| labelFilters['union'] = union; | ||
| } | ||
| let cacheKey = JSON.stringify({label: label.id}); |
There was a problem hiding this comment.
Better revert this to the previous behavior which just used the label ID as cache key. No need to stringify a whole object.
| if (!this.filtersCache.hasOwnProperty(cacheKey)) { | ||
| Vue.set(this.annotationsCache, cacheKey, []); |
There was a problem hiding this comment.
Should annotationsCache be modified here at all? If yes, is the cacheKey the correct one here? Before, only the label ID was used as the cache key.
Co-authored-by: Martin Zurowietz <mzur@users.noreply.github.com>
Co-authored-by: Martin Zurowietz <mzur@users.noreply.github.com>
Co-authored-by: Martin Zurowietz <mzur@users.noreply.github.com>
Co-authored-by: Martin Zurowietz <mzur@users.noreply.github.com>
…nto adds-shape-filter
|
@mzur About the JS coding style, do you have specific style guide that BIIGLE adheres to that I could learn more about and/or a suggestion for a formatter I can use as git prehook? I found |
|
I don't have a specific style guide to recommend. In biigle/core we have an eslint check but this is not very strict (mostly to spot errors). The only advice I can give is to adhere to the existing style of the code, whatever that is worth 🤷 |
This PR (partially) addresses issue #66 by introducing a filtering tab in Largo for users and shapes.
I also introduced tests for the new filtering functionalities on the backend side.
What I hoped to achieve, was a system that could be expanded upon. Please let me know if this is satisfactory.
Something that I am not 100% convinced is the fact that the filters get loaded on click - I was not able to pass the information from the API in any other way. Is it okay to you?
To test this, try to use filters to change the displayed largo annotations and check that no unexpected behaviour is present (e.g. annotations that should not be there that appear, other largo features stop working etc.)