ME-ICA / tedana

TE-dependent analysis of multi-echo fMRI
https://tedana.readthedocs.io
GNU Lesser General Public License v2.1
158 stars 94 forks source link

Fix dynamic reports with bokeh 3.4.0 problems #1068

Closed handwerkerd closed 2 months ago

handwerkerd commented 2 months ago

Closes #1063

I've now had 3 people ask about crashes related to this issue I'd like to figure out a nice solution to this problem.

Changes proposed in this pull request:

This no longer crashes, and the static report looks fine, but there is one annoying bug relating to the dynamic interactions. I'm not sure if this is due to the above changes or due to other changes in bokeh 3.4.0. The generated report looks fine, but if you click on one component and then another, then, instead of presenting the most recent one, both are highlighted and you have to unclick each component before highlighting a new one. I could use help from someone who has played with bokeh a bit more than I have (@eurunuela @javiergcas).

handwerkerd commented 2 months ago

This is an example of the new problem. We should either have one highlighted component or all highlighted. I sequentially clicked on 3 components and they all remained highlighted.

image
codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.69%. Comparing base (f084dc4) to head (75a7483).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1068 +/- ## ======================================= Coverage 89.68% 89.69% ======================================= Files 26 26 Lines 3482 3483 +1 Branches 615 615 ======================================= + Hits 3123 3124 +1 Misses 211 211 Partials 148 148 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

handwerkerd commented 2 months ago

I just ran this PR with bokeh=3.4.0 and bokeh=3.3.4. The above problem does NOT happen with v3.3.4, which means this the unaddressed issue is probably due to some other change within v3.4.0 and not the code changes in this PR.

eurunuela commented 2 months ago

I will have a look at it.

eurunuela commented 2 months ago

It is not as straightforward as I had hoped. I would suggest we limit bokeh to <=3.34 if the latest version isn't necessary for us.

handwerkerd commented 2 months ago

I decided to ask a question on their support forum. https://discourse.bokeh.org/t/dynamic-figure-problem-with-bokeh-v-3-4-0-udgrade/11416

If we don't hear about a solution in the next few days, I'll limit this PR to 3.3.4. Assuming no one else sees issues, my changes from circle to scatter are probably good to merge since they should be backwards compatible (I haven't tested v1.0.0, but scatter was added to that version) and will be issues in the future.

handwerkerd commented 2 months ago

I decided to look more carefully through all the PRs merged for v3.4.0. The one that stands out to me is Add support for xor selection and inversion by mattpap · Pull Request #13545 · bokeh/bokeh · GitHub It changed the default response for taps and the new behavior I’m seeing is vaguely similar to the new default. That is I now need to tap an element a second time to deselect it before clicking on a new element.

On the bokeh discussion board, I asked whether there a way to explicitly apply the previous default so that I can confirm whether or not this is the cause of our issue. They suggested I post that Q on another board, but I also wanted to share here and see if this might be something @eurunuela can already figure out how to do.

handwerkerd commented 2 months ago

I think I've solved this problem. The issue was that bokeh's model.TapTool default setting for mode changed from replace to xor. In practice, this meant the variable selected_idx previously was always a single number, but, in bokeh v3.4.0 was a string of numbers.

When defining toolbars, we just included the word tap instead of explicitly defining a model.TapTool. By adding models.TapTool(mode="replace") to a few places in the code, I think it now works. There was a second weird thing where the 'hover' icon was now appearing twice to the left of the scatter plot. I think that was because it was being differently defined in different functions. (if people have opinions on the order of those icons, we can rearrange)

handwerkerd commented 2 months ago

@tsalo and @eurunuela This is now ready for review. One think I want to check with you is that I've now added version numbers for bokeh to pyproject.toml. Given it has version numbers, will the dependabot automatically open new PRs when future bokeh versions are released or do we need to change something else as well?

handwerkerd commented 2 months ago

@eurunuela This is my first time playing around with the bokeh parts of this code. Could you take a look to make sure this all seems good to you?