Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Axis matching limitation #4501

Closed
nicolaskruchten opened this issue Jan 21, 2020 · 20 comments · Fixed by #4529
Closed

Axis matching limitation #4501

nicolaskruchten opened this issue Jan 21, 2020 · 20 comments · Fixed by #4529
Assignees
Labels
bug something broken

Comments

@nicolaskruchten
Copy link
Contributor

This is a pretty problematic case for Plotly.py: https://codepen.io/nicolaskruchten/pen/ExaOMRr?editable=true

In this case, all axes have matches set to the first axis (x or y) but those axes have no traces, so no information is being shared among the axes that do have traces.

I'd be tempted to call this a bug... thoughts @alexcjohnson @archmoj ?

This is a problem for Plotly.py because make_subplots creates subplots and sets matches before knowing which subplots have traces.

@alexcjohnson
Copy link
Collaborator

The issue is that xaxis and yaxis are included in layout but don't make it into fullLayout here, so they're not currently eligible to be used as matches='x' or 'y'. After some discussion, it seems that propagating them to fullLayout based on the fact that they're referenced by other axes - despite having no traces using them - is the preferred solution.

We also discussed matches='whatever' making a "match group," but this is problematic in that there may be settings in the lost axes (explicit range, for example) that would be lost using a match group solution.

A related question - though this part would clearly need to be a new feature, whereas the above may be considered a bug - is whether the trace-less subplots should show up in this case. One could argue that since xaxis.anchor = 'y' (and yaxis.anchor = 'x' but one seems like it should suffice) you should be able to at least opt in to showing the blank xy subplot.

@nicolaskruchten
Copy link
Contributor Author

@archmoj can you try the "add it to fullLayout" solution described above please, and let us know if any existing tests break or if from inspecting the code anything seems like it will break?

@archmoj
Copy link
Contributor

archmoj commented Jan 21, 2020

@nicolaskruchten sure.

@archmoj archmoj added the bug something broken label Jan 21, 2020
@archmoj archmoj self-assigned this Jan 21, 2020
@archmoj
Copy link
Contributor

archmoj commented Jan 21, 2020

Minimal bug demo.

@nicolaskruchten
Copy link
Contributor Author

I don't think this is the right bug... there is no matches in your pen?

@nicolaskruchten
Copy link
Contributor Author

the problem in my pen is that panning any subplot should pan all of them due to matches

@nicolaskruchten
Copy link
Contributor Author

A minimal test case would look like:

  • 3 subplots: same y but x, x2, x3
  • x2 and x3 have traces and matches x
  • x2 and x3 should pan together even when there are no traces on x

@alexcjohnson
Copy link
Collaborator

Yeah sorry, I muddied the waters with my "related question" about showing subplots with no traces. There may be another variant of the bug here too though: if you anchor an axis that is shown to another axis that isn't shown, will the one that is shown end up drawing its ticks etc in the wrong place? But let's keep that separate and just look at matches for now. I suspect this anchor variant could be more contentious, as it might have to convert the visible axis to free since it has no subplot to be drawn on.

@archmoj
Copy link
Contributor

archmoj commented Jan 21, 2020

Revised minimal bug demo.

@nicolaskruchten
Copy link
Contributor Author

I don't mind the current "no traces, no subplot shown" behaviour, so yes, let's factor that out of the discussion for now.

The core issue for me is that make_subplots in Python assumes that x2 and x3 matching x will link them together even if x has no traces.

@nicolaskruchten
Copy link
Contributor Author

Yes, the new minimal bug demo captures my intent here, thanks @archmoj :)

@archmoj
Copy link
Contributor

archmoj commented Jan 21, 2020

Not clear to me, if we should still pan together if the axes matches x2 and y2 references instead of x and y?
Here is the demo.

@nicolaskruchten
Copy link
Contributor Author

I don't understand that last question... can you rephrase?

@nicolaskruchten
Copy link
Contributor Author

To me, if A and B match C, they should pan together, regardless if C has traces, or if C's number is greater than or less than or in between A and B's numbers :)

@archmoj
Copy link
Contributor

archmoj commented Jan 21, 2020

In the minimal bug codepen, here I simply renamed x to x2 and y to y2 and used matching references to x2 and y2 instead of x and y. Do we still expect the axes to pan together, or x and y should be treated differently?

@nicolaskruchten
Copy link
Contributor Author

Yes, see above re A,B,C ... IMO there should be no special treatment for x/y per se as opposed to x2/y2 or whatever

@archmoj
Copy link
Contributor

archmoj commented Jan 28, 2020

Pasting comments from plotly_js channel here:

On Jan. 21

Mojtaba  9:43 AM

Linking subplots to the currently invalid axes appeared to be a pretty complicated process (with missing extremes to compute autorange, etc)
So I was not able to find a simple fix that we could release in a patch yesterday.
I suggest we possibly wait for Etienne to help with that.

Nicolas  10:07 AM

ok thanks for looking into it
did you consider my idea of substituting x2 for x etc?
(internally)
basically just interpreting x2 -> x <- x3 as x3 -> x2 if x is "invalid" ?

Mojtaba  10:19 AM

Yeah, I think that's the cleanest solution namely if we prefer not to display the x.y sub-plot in future.
Let me give that a second try.

@etpinard
Copy link
Contributor

Ok, let me try to summarise the current state of affairs.


Minimal example: https://codepen.io/MojtabaSamimi/pen/wvBRGyV?editors=0010

where xaxis2, xaxis3 and xaxis4 all have matches: 'x' which is currently invalid i.e. it isn't part of this list:

var allAxisIds = counterAxes.x.concat(counterAxes.y);

Proposed solution 1

From @alexcjohnson 's #4501 (comment)

After some discussion, it seems that propagating them to fullLayout based on the fact that they're referenced by other axes - despite having no traces using them - is the preferred solution.

@archmoj attempted this in 137d73d, but ran into problems trying to make this solution work, saying: "appeared to be a pretty complicated process (with missing extremes to compute autorange, etc)"

Proposed solution 2

From @nicolaskruchten 's https://plotly.slack.com/archives/C07MRPNHW/p1579705647015700

did you consider my idea of substituting x2 for x etc? basically just interpreting x2 -> x <- x3 as x3 -> x2 if x is "invalid" ?

This one is in PR #4506

As pointed out in #4506 (comment),

One thing this approach misses is if the lost axis sets an explicit range or rangemode https://codepen.io/alexcjohnson/pen/wvBNXgY?editors=0010
That seems like something we want to support, right?

and in #4506 (comment)

Also categoryarray and any other axis-related option I would guess. That is a drawback to this approach: you need to copy everything over basically


To me, solution 1 appears more robust, but also more complicated to implement.

But, it sounds like we could get solution 2 to work by finding the set of attributes that we need to "copy over" and by finding a way to remind us to potentially add attributes to that set whenever we add new cartesian axis features.

@archmoj would it be ok if I take a shot at implementing solution 1 and see just how "complicated" it gets?

@archmoj
Copy link
Contributor

archmoj commented Jan 28, 2020

@archmoj would it be ok if I take a shot at implementing solution 1 and see just how "complicated" it gets?

@etpinard Sure. Please go for it.

@archmoj
Copy link
Contributor

archmoj commented Jan 29, 2020

@etpinard
My dev branch is at: https://github.com/plotly/plotly.js/commits/fix4501-match-axes_dev

@archmoj archmoj removed their assignment Jan 29, 2020
@etpinard etpinard self-assigned this Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
4 participants