#16 - Add roi tool #28

niksirbi · 2023-01-12T13:25:22Z

In this PR I will tackle issue #16

codecov-commenter · 2023-01-12T14:59:53Z

Codecov Report

Merging #28 (5df5991) into main (d3e0ee7) will decrease coverage by 3.91%.
The diff coverage is 7.32%.

@@            Coverage Diff            @@
##             main     #28      +/-   ##
=========================================
- Coverage   13.28%   9.38%   -3.91%     
=========================================
  Files           4       4              
  Lines         143     373     +230     
=========================================
+ Hits           19      35      +16     
- Misses        124     338     +214

Impacted Files	Coverage Δ
wazp/app.py	`0.00% <0.00%> (ø)`
wazp/callbacks.py	`0.00% <0.00%> (ø)`
wazp/utils.py	`32.98% <25.75%> (-17.02%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

… alerts

niksirbi · 2023-02-27T14:57:30Z

This PR closes issue #16
Apologies for the gigantic PR (and to think that this is the cleaned-up, rebased, version).
So this is how the ROI tab currently looks like:

Features

The user selects a video from the dropdown (this is auto-populated based on the uploaded project config)
The video selection triggers an update of the frame slider. The user may select one of 5 frames, equally spaced across the video (the default is the middle frame).
Whenever a video or a frame is changed, the corresponding frame will be shown on the graph below. Frames used within the last day are cached locally (this saves time, given that extracting a frame from a video may take a few seconds).
The user may draw ROIs by first selecting the appropriate ROI from the dropdown (auto-populated based on the project config) and then drawing in freeform (specifically using the drawclosedpath tool, see discussion under issue Improve definition of ROI #11 )
The drawn ROIs automatically appear on the table and are also saved in the browser session (this means that if a user goes to another video and comes back later, the drawn ROIs will not be lost)
ROIs can be edited by selecting them and editing/deleting individual nodes, or via translating the entire ROI. A selected ROI can also be deleted. All edits on the graph automatically appear on the table as well.
The video frame on which the ROI was last drawn/edited is kept in the session storage and also appears on the table.
The user can save ROIs to the video's .metadata.yaml file. This creates a new ROIs field which has the following form:

ROIs:
- drawn_on_frame: 57000
  line_color: '#2E91E5'
  name: enclosure
  path: M348.0491395793499,...
- drawn_on_frame: 38000
  line_color: '#E15F99'
  name: left_box
  path: M484.23300516875133,...

The user may also load the ROIs from a video's metadata file. The uploaded info will overwrite whatever is on the graph and the table.

What is missing

The "infer ROIs" button is currently not-functional (to be implemented in the future, see issues Improve definition of ROI #11 and Common coordinate system for videos #3)
The user may currently draw self-intersecting polygons as ROIs (thedrawclosedpath doesn't prevent that). We may want to implement a check for the validity of a polygon as an ROI
Adding the ROIs field to the metadata files breaks the table-building functionalities of the metadata tab. I tried to solve this in various ways without success. Currently, the issue is circumvented by building the metadata table from fields that are present in all metadata files (by specifying join="inner"). In the future, we should think of how exactly to represent the presence/absence of defined ROIs in the metadata files and in the metadata table. Some drastic solutions would be to not show the ROis in the metadata table or to save the ROIs to a separate file (but perhaps a non-drastic solution can be found).

niksirbi · 2023-02-27T18:52:23Z

Note on the above

To really review this PR, you need to alias (at least some of) the videos in sample_project to a location on your system that actually contains the data (e.g. on a mounted /cep/zoo drive). At least until we find a more permanent solution, as per issue #33

samcunliffe · 2023-02-28T12:15:47Z

Manual testing looks good. I've just drawn some boxes. I can see the paths appear in the metadata.yaml 👍 I can also (re)load them on closing and restarting the server.

How easy would it be (sorry!) to implement backspace deletion? Moving the RoIs around and resizing are both possible, but my naive attempts to delete (backspace and right-click) don't do anything, I have to notice the feint backspace icon on hover at the top.

If this is not easy, please ignore. The dash "hovering toolbar" interface is adequate. And anyway the researchers will become accustomed to this because the plots also have that.

samcunliffe

LGTM!

The only substantial thing is my backspace comment ☝️.

.github/workflows/test_and_deploy.yml

wazp/callbacks.py

wazp/utils.py

sample_project/input_config.yaml

niksirbi · 2023-03-01T13:29:00Z

How easy would it be (sorry!) to implement backspace deletion? Moving the RoIs around and resizing are both possible, but my naive attempts to delete (backspace and right-click) don't do anything, I have to notice the feint backspace icon on hover at the top.

I actually thought of that as well. Unfortunately, I haven't found any plotly-native way of enabling shape deletion via clicking delete or backspace. The canonical way of doing that is through the "erase" tool on the toolbar above the graph. We could design custom callbacks, but I didn't want to complicate things.

An idea would be to add a delete functionality to rows of the table, such that if you delete a row, the corresponding shape disappears. That would be possible, but would increase the complexity of the callback dance between frame-graph, roi-storage, and roi-table.

sfmig

Looks good!

Mostly minor comments post-hoc. The only comment that I think would be more important is the one with suggestions on the layout. I think right now there is a lot of back an forth between different areas of the screen (visually and with the mouse). I added some suggestions to hopefully make it flow a bit more naturally.

sfmig · 2023-03-02T14:33:14Z

sample_project/input_config.yaml

@@ -3,3 +3,10 @@ metadata_fields_file_path: ./sample_project/metadata_fields.yaml
 metadata_key_field_str: File
 pose_estimation_model_str: DLC_resnet50_jwasp_femaleandmaleSep12shuffle1_1000000
 pose_estimation_results_path: ./sample_project/pose_estimation_results
+roi_names:


minor consistency suggestions:

shall we call these 'ROI_tags', to match the 'event_tags'?

shall we always write ROI in caps?

sfmig · 2023-03-02T14:41:14Z

sample_project/videos/jwaspE_nectar-open-close_control.metadata.yaml

+Hardware_description: Reference 'nectar-open-close.pdf' on ceph (in \zoo\raw\LondonZoo\Experiment-protocols\Jewel-wasp_Ampulex-compressa\)
+  or Google Drive (most up-to-date version if they are different - in Experiment Protocols\jewel-wasp_Ampulex-compressa\)
+ROIs:
+- drawn_on_frame: 57000


When you dump a dictionary in a yaml file, by default the keys are sorted alphabetically. The order of the keys is preserved if you add the sort_keys=False argument (so yaml.safe_dump(<dict>, <yaml_file>, sort_keys=False)).

A small thing, but if the metadata files are to be read or checked by a human, I think it would be nice to show the keys in a more intuitive order (maybe name > drawn_on_frame > path > line_color). Also relevant for the visualisation in the metadata table.

also we need to add the ROIs field to the metadata_fields.yaml (otherwise for me the tests fail)

sfmig · 2023-03-02T14:45:42Z

wazp/callbacks.py


 VIDEO_TYPES = [".avi", ".mp4"]
 # TODO: other video extensions? have this in project config file instead?
+ROI_CMAP = px.colors.qualitative.Dark24
+# TODO: make colomap this a project config parameter?


I'm in favor of keeping the project config minimal (that's why I was doubting about adding the VIDEO_TYPES variable above).

One option may be to have sections within the config file, in which we can separate 'cosmetic' parameters from mandatory fields (not sure what would be the best way to do this though, maybe just with comments?).

In any case I think the ROIs colormap won't be something the user will want to change, so I would say for this there is no need to add it to the project config.

sfmig · 2023-03-02T17:17:21Z

wazp/callbacks.py

+                ]
+            video_paths.sort()
+            video_names = [p.name for p in video_paths]
+            video_paths_str = [p.absolute().as_posix() for p in video_paths]


thinking about refactoring: we may want to have this video paths extraction as a helper function, we use it both quite a bit

sfmig · 2023-03-02T17:20:20Z

wazp/callbacks.py

+        ],
+        Input("session-storage", "data"),
+    )
+    def update_roi_select_options(


I like the consistency in the function naming....taking notes for refactoring phase 🤓

sfmig · 2023-03-02T17:42:15Z

wazp/callbacks.py

+    ) -> list:
+        """
+        Set the color of the ROI names in the ROI table
+        based on the color assigned to that ROI shape.


I'm not sure if I am biased but I find this description very copilot-y! haha 😁

Shouldn't it just be 'Set the color of the ROIs in the table'?

sfmig · 2023-03-02T17:45:11Z

wazp/callbacks.py

+        roi_color_mapping : dict
+            Dictionary with the following keys:
+                - roi2color: dict mapping ROI names to colors
+                - color2roi: dict mapping colors to ROI names


probably irrelevant but I don't understand why you need a color2roi mapping... 🤔

sfmig · 2023-03-02T17:47:43Z

wazp/callbacks.py

+
+        # If triggered by the load ROIs button click
+        # Load the ROIs from the metadata file
+        # CAUTION! This will overwrite any ROIs is the roi-storage


typo: *in the roi-storage

sfmig · 2023-03-02T17:49:18Z

wazp/callbacks.py

+            State("roi-colors-storage", "data"),
+        ],
+    )
+    def update_roi_storage(


I like that this function has nice guiding comments

sfmig · 2023-03-02T18:18:39Z

wazp/pages/02_ROI.py

+# Initialize the ROI status alert
+init_roi_status: dict = {"message": "No ROIs to save.", "color": "light"}
+
+# Instructions for the user


Some thoughts about the layout:

It'd be nice if the layout was intuitive enough so that we can omit written instructions. If we see we do need them, one option to make them take less space could be to put them in a collapsable panel (just as we were thinking to show the code that generates plots).

I find it more user friendly if all the inputs are on the same area. In this case, the selected video, the selected frame, and the ROI to label next. In that spirit I would suggest to move the 'Create new ROI for' just under the frame selector. Otherwise, I feel there is a lot of back and forth across the screen.

similarly, I think the 'Save ROIs' button could be better placed under the table. After you define them, you review the table and then click save. I think it follows a bit more naturally the workflow. I'd keep the other buttons together with the 'Save ROIs' one, to have all the actions together.

niksirbi mentioned this pull request Jan 12, 2023

Improve definition of ROI #11

Open

niksirbi linked an issue Jan 12, 2023 that may be closed by this pull request

implement a simple ROI configuration per video #16

Closed

samcunliffe mentioned this pull request Feb 14, 2023

Also test in python 3.11. #31

Merged

niksirbi added 24 commits February 16, 2023 11:14

added numpy and pillow dependencies

2fd7880

added dash-player as dependency

d9b961e

added dash-daq as dependency

bf254c8

added opencv-python as dependency

1c6a6be

added roi names to input_config

a837228

added type-Pillow as dev dependency

c032597

mypy ignores cv2 but checks wazp files

11eaed0

updated github action versions

5d4781f

initialized basic ROI page layout showing empty video frame

d2a4297

added layouts for roi table and dropdown

cf588bc

added video selection dropdown to frame card header

36c6bac

update video select dorpdown via a callback

b317cd3

added callback for updating ROI dropdown and assigning an ROI colormap

f315828

added frame number input field to the frame graph header

cf1db4e

added callback for updating max frame number depending on video

11060f9

added status alerts to frame graph and roi table footers

bdb4678

added element for storing ROIs

b3c36cc

added utility functions for converting between ROIs in graph and table

fe5262d

added utility for extracting frame from video

feec6a9

added callbacks for updating frame-graph and roi table

ddac3c9

added callback for updating ROI colors in table

5ae53fc

added button for saving/loading ROIs

b8fd273

added callback for saving rois and update roi status alert

02807e2

added missing docstrings

ab57a6e

niksirbi force-pushed the add-roi-tool branch from e136772 to ab57a6e Compare February 16, 2023 16:12

make non-implemente button inactive and visually distinct

ecd22df

niksirbi added 15 commits February 17, 2023 14:59

corrected docstring for update_frame_slider callback

c392323

untangled messy ROI callbacks and associated utils

debfd72

raise alert if frame cannot be found/extracted

83754d9

display instructions for drawing ROIs

1f89bda

updated text of alert message under the frame

ff91ad8

added functionalities for saving and loading ROIs to metadata yaml

95f5f8f

edited instructions below the frame graph

601b7fa

make sure the correct roi status is displayed on the table and in the…

fea0683

… alerts

cached frames older than 1 day are cleaned

2fbc766

videos with empty ROI storage also show an empty ROI table

b437ad5

fixed frame slider

8906388

updated frame and roi status alerts

3e87f11

added loading spinners to inform user about time-consuming steps

bfc2d73

roi-storage is saved for the session

9ed5f59

excluided sample_project .avi and .h5 files from manifest

8a9013b

niksirbi force-pushed the add-roi-tool branch from 9aec5cc to 8a9013b Compare February 27, 2023 14:18

niksirbi marked this pull request as ready for review February 27, 2023 15:02

niksirbi requested review from sfmig and samcunliffe February 27, 2023 15:02

samcunliffe approved these changes Feb 28, 2023

View reviewed changes

explicitly refer to full objects in docstrings (for uncommon libraries)

5df5991

niksirbi mentioned this pull request Mar 1, 2023

add ROI in the exported dataframe #40

Closed

niksirbi merged commit 3e52266 into main Mar 2, 2023

sfmig reviewed Mar 2, 2023

View reviewed changes

sfmig mentioned this pull request Mar 2, 2023

small additions to ROI tab #44

Closed

niksirbi deleted the add-roi-tool branch April 3, 2023 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#16 - Add roi tool #28

#16 - Add roi tool #28

niksirbi commented Jan 12, 2023

codecov-commenter commented Jan 12, 2023 •

edited

Loading

niksirbi commented Feb 27, 2023

niksirbi commented Feb 27, 2023

samcunliffe commented Feb 28, 2023

samcunliffe left a comment

niksirbi commented Mar 1, 2023

sfmig left a comment

sfmig Mar 2, 2023

sfmig Mar 2, 2023

sfmig Mar 2, 2023

sfmig Mar 2, 2023

sfmig Mar 2, 2023

sfmig Mar 2, 2023

sfmig Mar 2, 2023

sfmig Mar 2, 2023

sfmig Mar 2, 2023

sfmig Mar 2, 2023

sfmig Mar 2, 2023

#16 - Add roi tool #28

#16 - Add roi tool #28

Conversation

niksirbi commented Jan 12, 2023

codecov-commenter commented Jan 12, 2023 • edited Loading

Codecov Report

niksirbi commented Feb 27, 2023

Features

What is missing

niksirbi commented Feb 27, 2023

Note on the above

samcunliffe commented Feb 28, 2023

samcunliffe left a comment

Choose a reason for hiding this comment

niksirbi commented Mar 1, 2023

sfmig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 12, 2023 •

edited

Loading