fix for spikedistance set to infinity #4627

antoinerg · 2020-03-11T17:41:00Z

Commit 9d1325c adds a test which fail.

For #4626:
Before: https://codepen.io/antoinerg/pen/BaNYJve
After: https://codepen.io/antoinerg/pen/oNXEENy

For #3805:
Before: https://codepen.io/anon/pen/eoPXJE
After: https://codepen.io/antoinerg/pen/ExjQQYw

antoinerg · 2020-03-11T17:58:10Z

src/components/fx/hover.js

@@ -549,7 +549,7 @@ function _hover(gd, evt, subplot, noHoverEvent) {
        var thisSpikeDistance;
        for(var i = 0; i < pointsData.length; i++) {
            thisSpikeDistance = pointsData[i].spikeDistance;
-            if(thisSpikeDistance < minDistance && thisSpikeDistance <= spikedistance) {
+            if(thisSpikeDistance <= minDistance && thisSpikeDistance <= spikedistance) {


The problem stems from thisSpikeDistance being Infinity and minDistance also being Infinity. Without an equal sign, this will never be true and no spikelines will appear hence the bug.

Are there any cases this changes which point we get spikes to? It might still be OK in that case, I just want to know what those cases are. The loop indicates this way if we have multiple items in pointData all with the same spikeDistance, previously we'd get the first one, but with the new logic we'll get the last one.

Are there any cases this changes which point we get spikes to?

@alexcjohnson I'm not sure how to proceed to find out.

Should we check whether minDistance is Infinity and handle that case separately?

That's a much better solution, IMO.

if((minDistance === Infinity || thisSpikeDistance < minDistance) && thisSpikeDistance <= spikedistance ) {

looks like the right logic to apply.

Ok so I went with this safer approach. If spikedistance is Infinity we visit that if block. It should preserve any existing behavior but fix the edge case for Infinity.

See 583ad64

The key is to first identify the relevant test cases. I suspect the relevant ones are (with hovermode: 'x'):

Two scatter traces with exactly identical points, so there aren't closer and farther ones. Maybe we don't care which one gets the spike line... though offhand I'd suggest the last one, since it'll be drawn on top so you'll see a spike line color matching the visible point.

Stacked bar traces with the cursor at a larger (or smaller) y value than any of the bars. Ideally I'd think it would be nice to draw the spike line to the one that's closest; but that would require getting away from using Infinity for all points outside the bar, at least for sizeFn. Perhaps we give Fx.inbox another (optional) argument failVal that, in this case, will be set very large, and increasing the farther away you get from the bar either above or below, but not infinite. Or, at that point it seems silly to use Fx.inbox at all, just inline this logic.

Thanks for the clarification @alexcjohnson . I now better understand the purpose of this code. I will take another approach.

archmoj · 2020-03-11T18:21:59Z

@antoinerg Thanks very much for the PR.
Looks good to my eyes.
I'll let a second reviewer to give you dancers.

archmoj · 2020-03-11T22:00:22Z

Nicely done.
💃

archmoj · 2020-03-11T22:03:12Z

Please hold on. There is a new comment: #4627 (comment).

antoinerg · 2020-03-12T01:22:23Z

I made a mess earlier and cleaned up the git history with a git rebase. I left commit ae7db42 with the failing test to fix and made a new solution in ebf75e6 .

P.S. I updated the Codepen above to run the latest build.

antoinerg · 2020-03-12T22:12:25Z

I think my first solution has to go back in otherwise we have too many non-scatter traces for which no spike lines appear for spikedistance: -1.

@alexcjohnson can you have a look at branch fix-spikedistance. It contains new tests which make sure that:

all cartesian traces produce spike lines when spikedistance: -1 (first commit)
test that spikeline is drawn up to the last point as you described
test that spikeline is drawn up to the closest bar

If it's satisfactory, I can open a new PR or simply force push here. Let me know what you prefer.

antoinerg · 2020-03-12T22:13:24Z

@archmoj you might also want to review my new branch described in #4627 (comment)

alexcjohnson · 2020-03-12T23:34:16Z

@alexcjohnson can you have a look at branch fix-spikedistance. It contains new tests...

Looks great, my only question about it is the test "correctly select the closest bar even when setting spikedistance to -1" - you do _hover({xpx: 600, ypx: 400}); and _hover({xpx: 580, ypx: 200});, which look like they're within the bounds of the blue and green bars at E? That's important (though it would be clearer with exactly the same xpx), but I would have expected to also see ypx corresponding to positions not on any bars, but above and below the entire stack - which would probably require increasing the range to create space above and below. I'm not sure if that works right now but I think the goal of #3805 would be that if you get a hover point from a bar, and you set spikedistance=-1, you should get a spike line to the closest one.

archmoj · 2020-03-13T00:39:41Z

If it's satisfactory, I can open a new PR or simply force push here. Let me know what you prefer.

Force-pushing on PRs is discouraged on the plotly.js repo.
Please simply add the commit to this PR or open a new PR.
Thank you!

antoinerg · 2020-03-13T03:30:31Z

my only question about it is the test "correctly select the closest bar even when setting spikedistance to -1" - you do _hover({xpx: 600, ypx: 400}); and _hover({xpx: 580, ypx: 200});, which look like they're within the bounds of the blue and green bars at E?

I'm not completely sure: maybe it's above or below. I tested it manually on mock bar_stack-with-gaps to make sure we get the closest one.

That's important (though it would be clearer with exactly the same xpx)

Yes they should have the same xpx, sorry for making things confusing. I will fix that.

but I would have expected to also see ypx corresponding to positions not on any bars, but above and below the entire stack - which would probably require increasing the range to create space above and below. I'm not sure if that works right now

Right now, for bar traces, spike lines will appear only if you hover inside the bar itself for any value spikedistance > 0 (this is also the case on master). My PR fix-spikedistance only fixes the case spikedistance = -1 which is key for unified hover label. I'm not sure if I have time to tackle the other case (ie spikedistance > 0 ) at the moment.

if you get a hover point from a bar, and you set spikedistance=-1, you should get a spike line to the closest one.

I think this case is covered: https://codepen.io/antoinerg/pen/vYORyLV

cc @alexcjohnson

alexcjohnson · 2020-03-13T12:56:17Z

You're so close! Right now you get a spike to the last bar when you're off the stack, not the closest one (in the gif below see behavior when the mouse is below, spike goes to the green bar). fixing that would require tweaking sizefn to include some large-but-not-infinite value when off the bar, that grows larger as you get farther away.

This is not a super important case, so if you want to move on that's fine, but if you do please make a new issue for this.

antoinerg · 2020-03-13T14:36:42Z

You're so close! Right now you get a spike to the last bar when you're off the stack, not the closest one (in the gif below see behavior when the mouse is below, spike goes to the green bar). fixing that would require tweaking sizefn to include some large-but-not-infinite value when off the bar, that grows larger as you get farther away.

This is not a super important case, so if you want to move on that's fine, but if you do please make a new issue for this.

I missed that behavior! Let me see if I can get it right :) Thanks for the review @alexcjohnson 🕵️‍♂️

EDIT: I am not sure I can at the moment. I don't understand the current calculation of spikedistance and what it is supposed to represent. The equations make no sense to me. I'll open an issue and revisit later if time permits.

antoinerg · 2020-03-13T18:55:13Z

Please simply add the commit to this PR or open a new PR.

I'll open a new PR. Closing in favor of #4637

antoinerg commented Mar 11, 2020

View reviewed changes

archmoj added status: reviewable bug something broken labels Mar 11, 2020

archmoj requested a review from alexcjohnson March 11, 2020 18:22

antoinerg mentioned this pull request Mar 11, 2020

unified hover label #4620

Merged

4 tasks

spikeline: test spikedistance -1 on a bar trace

ae7db42

antoinerg force-pushed the fix-spikeline-infinity branch 2 times, most recently from 7e787ad to 9a0143f Compare March 12, 2020 00:57

bar hover: fix spikedistance calculation

ebf75e6

antoinerg force-pushed the fix-spikeline-infinity branch from 9a0143f to ebf75e6 Compare March 12, 2020 01:04

antoinerg added this to the v1.53.0 milestone Mar 12, 2020

antoinerg self-assigned this Mar 12, 2020

antoinerg mentioned this pull request Mar 13, 2020

fix for spikedistance=-1 #4637

Merged

antoinerg closed this Mar 13, 2020

antoinerg mentioned this pull request Mar 13, 2020

spikedistance=-1: lines are not drawn up to the closest bar in some cases #4638

Closed

antoinerg deleted the fix-spikeline-infinity branch April 3, 2020 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for spikedistance set to infinity #4627

fix for spikedistance set to infinity #4627

antoinerg commented Mar 11, 2020 •

edited

Loading

antoinerg Mar 11, 2020 •

edited

Loading

alexcjohnson Mar 11, 2020

antoinerg Mar 11, 2020

antoinerg Mar 11, 2020

archmoj Mar 11, 2020

archmoj Mar 11, 2020

antoinerg Mar 11, 2020

alexcjohnson Mar 11, 2020

antoinerg Mar 12, 2020

archmoj commented Mar 11, 2020

archmoj commented Mar 11, 2020

archmoj commented Mar 11, 2020

antoinerg commented Mar 12, 2020 •

edited

Loading

antoinerg commented Mar 12, 2020

antoinerg commented Mar 12, 2020

alexcjohnson commented Mar 12, 2020

archmoj commented Mar 13, 2020

antoinerg commented Mar 13, 2020

alexcjohnson commented Mar 13, 2020

antoinerg commented Mar 13, 2020 •

edited

Loading

antoinerg commented Mar 13, 2020

fix for spikedistance set to infinity #4627

fix for spikedistance set to infinity #4627

Conversation

antoinerg commented Mar 11, 2020 • edited Loading

antoinerg Mar 11, 2020 • edited Loading

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

archmoj commented Mar 11, 2020

archmoj commented Mar 11, 2020

archmoj commented Mar 11, 2020

antoinerg commented Mar 12, 2020 • edited Loading

antoinerg commented Mar 12, 2020

antoinerg commented Mar 12, 2020

alexcjohnson commented Mar 12, 2020

archmoj commented Mar 13, 2020

antoinerg commented Mar 13, 2020

alexcjohnson commented Mar 13, 2020

antoinerg commented Mar 13, 2020 • edited Loading

antoinerg commented Mar 13, 2020

antoinerg commented Mar 11, 2020 •

edited

Loading

antoinerg Mar 11, 2020 •

edited

Loading

antoinerg commented Mar 12, 2020 •

edited

Loading

antoinerg commented Mar 13, 2020 •

edited

Loading