Dash annotating

[scott-huberty]

Closes #31 Closes #66

Updated the annotating method in dashboard. This drops the dependency for dash-extension and does not require a keyboard press.

You can now modify and delete bad annotations in teh dashboard (before, you could only make new ones) (Enhancement).

TODOS:

• [x] Sync annotations across ICA and EEG visualizers. (need to redo this after my refactor).
• [x] Codespell Flake & docstring

[christian-oreilly]

@scott-huberty Note that your choice of using the visual element (shapes) to hold the annotation data will cause you issues. E.g., when an annotation is at the border of the visualized region, you will have either to show the whole annotations which will cause the xlim and ylim of the graph to be changed to fit the shape into the graph (I think this might be what cause the white margins appearing on this version) or you will have to crop the shape and keep somewhere else the information about the real start-stop of that annotation (not the cropped values). I think, in general, it would be better to keep the raw objects being the holder of the annotation data and the shapes just be whatever we need to display at a given moment.

On a different topic, another possible refactoring is to create a new class (something like DashAnnot) that would wrap the two objects that are currently being dealt separately for the shape and the description.

Also, sync_time_sliders can be split in two callbacks, one per slider. You could event have it dry I think... I think this would work:

        slider_ids = [self.eeg_visualizer.dash_ids['time-slider'], 
                   self.ica_visualizer.dash_ids['time-slider']]
        for this_slider_id, other_slider_id in zip(slider_ids, slider_ids[::-1]):
            @self.app.callback(
                    Output(other_slider_id, 'value'),
                    Output(other_slider_id, 'min'),
                    Output(other_slider_id, 'vmax'),
                    Input(this_slider_id, 'value'),
                    Input('file-dropdown', 'placeholder'),
                    prevent_initial_call=True)
            def sync_time_sliders(time_slider, selected_file):
                ctx = dash.callback_context
                if ctx.triggered:
                    trigger_id = ctx.triggered[0]["prop_id"].split(".")[0]
                    if trigger_id == this_slider_id:
                        return time_slider, no_update, no_update
                    if trigger_id == 'file-dropdown':
                        value = self.ica_visualizer.time_slider.value
                        min_ = self.ica_visualizer.time_slider.min
                        max_ = self.ica_visualizer.time_slider.max
                        return value, min_, max_

[scott-huberty]

I think this might be what cause the white margins appearing on this version

If this is what you are talking about, it has always happened (this screenshot is from main).

I think we need to fix the xaxis range, similar to how we fix the range of the yaxis.

https://github.com/lina-usc/pylossless/blob/13d7523fd3a4b7bc136aec52cb6b00836e0c8423/pylossless/dash/mne_visualizer.py#L199-L207

using the visual element (shapes) to hold the annotation data .... when an annotation is at the border of the visualized region, you will have either to show the whole annotations which will cause the xlim and ylim of the graph to be changed to fit the shape into the graph ... or you will have to crop the shape and keep somewhere else the information about the real start-stop of that annotation (not the cropped values).

Yeah... I was thinking about this too...

I think, in general, it would be better to keep the raw objects being the holder of the annotation data and the shapes just be whatever we need to display at a given moment.

Let me know if you have any ideas on how this could be done.

On a different topic, another possible refactoring is to create a new class (something like DashAnnot) that would wrap the two objects that are currently being dealt separately for the shape and the description.

Interesting.. might be a good idea!

[scott-huberty]

note that your choice of using the visual element (shapes) to hold the annotation data will cause you issues. ... when an annotation is at the border of the visualized region ... cause the xlim and ylim of the graph to be changed to fit the shape into the graph (I think this might be what cause the white margins appearing on this version)

I just commited a fix. Fixing the xaxis range to the tmin and tmax, fixes both the issue of a shape that spans beyond the time-window, and fixes the issue where there was white space at the beginning and end of the timeseries (screen shot above; note this was NOT an issue unique to this branch. It also occurs on main and has always occured).

[christian-oreilly]

note this was NOT an issue unique to this branch. It also occurs on main and has always occured

Funny, I've never noticed that. Maybe we were too busy with other stuff ;)

[scott-huberty]

Note that we are not quite done with our time slider work... While the value, min and max are refreshed, we need to refresh the ticklabels of the time slider! If you manually refresh the page the tick labels show back up on the time slider (they are supposed to be there to help the user navigate the time).

[christian-oreilly]

The ticklabels thing seems trivial to me now that we know what was the way to solve the min and max... we just need to add another output for marks, no? I'd guess something like:

        slider_ids = [self.eeg_visualizer.dash_ids['time-slider'], 
                   self.ica_visualizer.dash_ids['time-slider']]
        for this_slider_id, other_slider_id in zip(slider_ids, slider_ids[::-1]):
            @self.app.callback(
                    Output(other_slider_id, 'value'),
                    Output(other_slider_id, 'min'),
                    Output(other_slider_id, 'vmax'),
                    Output(other_slider_id, 'marks'),
                    Input(this_slider_id, 'value'),
                    Input('file-dropdown', 'placeholder'),
                    prevent_initial_call=True)
            def sync_time_sliders(time_slider, selected_file):
                ctx = dash.callback_context
                if ctx.triggered:
                    trigger_id = ctx.triggered[0]["prop_id"].split(".")[0]
                    if trigger_id == this_slider_id:
                        return time_slider, no_update, no_update
                    if trigger_id == 'file-dropdown':
                        value = self.ica_visualizer.time_slider.value
                        min_ = self.ica_visualizer.time_slider.min
                        max_ = self.ica_visualizer.time_slider.max
                        marks = self.ica_visualizer.time_slider.marks
                        return value, min_, max_, marks

[christian-oreilly]

@scott-huberty I just pushed a fix for that (and DRYer code for sliders callback) on the usc-lica:dash_annotating branch.

[scott-huberty]

we just need to add another output for marks, no

Yeah exactly. Thanks for doing that!!

[scott-huberty]

Work is done. I will merge this tomorrow.

[christian-oreilly]

@scott-huberty You did not address a review request. Also, please give me time to do an overall review before merging; this is a substantial PR.

[scott-huberty]

@christian-oreilly

No prob. Assigned you as reviewer.

[christian-oreilly]

I thought I already was! Anyway, thanks. Have a look at my earlier comment/review. I should be able to give a complete review by the end of the day, so I won't hold this forever...

[scott-huberty]

my earlier comment/review.

I saw (and responded to) your earlier comments about refactoring the annot store into a class / wish to keep the annots in the raw object - but I don't see any requested changes on the code (but let me know if I'm missing something)

[christian-oreilly]

Damn. It is the second time I make this mistake. The review was not submitted. It was like a draft. It should be submitted now.

[christian-oreilly]

Very nice to have this one merged.