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

Implementing matching axes #3506

Merged
merged 23 commits into from Feb 18, 2019

Conversation

Projects
None yet
5 participants
@etpinard
Copy link
Member

etpinard commented Feb 1, 2019

resolves #1549 - with a new cartesian axis attribute ax.matches ❗️


Demos:

Matching x, x2 and x3 axes:

peek 2019-02-01 17-26

Matching splom axes (notice the constrained zoombox on the diagonal):

peek 2019-02-01 17-28

Or funkier things, like matching an overlaying axis:

peek 2019-02-01 17-27


Notable features:

  • matching axes get matching autorange values 6f2bb78, c5f9e74
  • matching axes get matching categories d9c2d4e
  • matching axes get matching autobins 9118505
  • a new splom dimension attribute (axis.matches) to turn on matching axes behavior for splom-generated axes c290adf
  • guaranteed fun when dragging around subplots with matching axes

More technically, I attempted to reuse most of the scaleanchor logic, for better and worse as some things didn't match (pun 😏 ) too well. I also cleaned up a few things in the cartesian_interact_test.js suite (in 13007e0, e962e96 ca1de5d)

I'll tag this thing as reviewable, but I'll hunt around for edge cases again on Monday. I'll not 100% convinced the scaleanchor + matches mock (added in 8c09944) does the right thing (update: won't be done in this PR). Moreover, I'll try to incorporate #3460 into this branch (done in: 99a9edb)

cc @plotly/plotly_js


group = traceOut._groupName = isOverlay ? traceOut.uid : (
getAxisGroup(fullLayout, traceOut.xaxis) +
getAxisGroup(fullLayout, traceOut.yaxis) +

This comment has been minimized.

Copy link
@etpinard

etpinard Feb 1, 2019

Author Member

N.B. currently, both x and y need to match in order to be considered in the same auto-bin group.

@etpinard

This comment has been minimized.

Copy link
Member Author

etpinard commented Feb 1, 2019

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

alexcjohnson commented Feb 2, 2019

This is awesome @etpinard, so much fun to play with!

I'll not 100% convinced the scaleanchor + matches mock (added in 8c09944) does the right thing.

yeah, that one has some problems... looks like after a zoom/pan it manages to satisfy all the constraints (the very shrunken subplots I think are correct), but initially or after an autorange it does not.
axes_scaleanchor-with-matches

Here's a more extreme example. Kind of a pathological case, where each subplot has x&y matching, and the x of each subplot is supposed to scale with the y of the previous... this is the case I think compounds height/width differences, so it should be expected that either the first or last subplot gets its data very compressed if you stretch it to a very non-square size. Right now it appears to apply all the scaleanchor constraints, but all the matches constraints except the last are lost.
screen shot 2019-02-02 at 8 22 00 am

Plotly.newPlot(gd,[
  {y:[1,2]},
  {y:[1,2],xaxis:'x2',yaxis:'y2'},
  {y:[1,2],xaxis:'x3',yaxis:'y3'},
  {y:[1,2],xaxis:'x4',yaxis:'y4'}
], {
  yaxis: {matches: 'x'},
  xaxis2: {scaleanchor: 'y'},
  yaxis2: {matches: 'x2'},
  xaxis3: {scaleanchor: 'y2'},
  yaxis3: {matches: 'x3'},
  xaxis4: {scaleanchor: 'y3'},
  yaxis4: {matches: 'x4'},
  grid: {rows: 2, columns: 2, pattern: 'independent'},
  width: 500,
  height: 400
})

I haven't looked at your code in detail yet, but in order to satisfy this kind of chained constraints seems to me it may be necessary to generate scaleanchor constraints based on each matches constraint (using gs._w and gs._h) and use that to run through the scaleanchor algorithm, and only after that apply the matches constraints explicitly.

@etpinard

This comment was marked as outdated.

Copy link
Member Author

etpinard commented Feb 4, 2019

TODO:

- [ ] match histogram2d binning of matching axes

EDIT: now in #3515

@etpinard

This comment has been minimized.

Copy link
Member Author

etpinard commented Feb 5, 2019

@alexcjohnson here's an attempt at satisfying both matches and scaleanchor: 74cf76c

In that commit, matching autorange:true axes first have their range value computed then we enforce the scaleanchor constraint as if their range was set. This way, only their domain is tweaked during enforceAxisConstraints. I'm not 100% sure this is what users expect, but it seems to make things work ok.

Now, about your

Plotly.newPlot(gd,[
  {y:[1,2]},
  {y:[1,2],xaxis:'x2',yaxis:'y2'},
  {y:[1,2],xaxis:'x3',yaxis:'y3'},
  {y:[1,2],xaxis:'x4',yaxis:'y4'}
], {
  yaxis: {matches: 'x'},
  xaxis2: {scaleanchor: 'y'},
  yaxis2: {matches: 'x2'},
  xaxis3: {scaleanchor: 'y2'},
  yaxis3: {matches: 'x3'},
  xaxis4: {scaleanchor: 'y3'},
  yaxis4: {matches: 'x4'},
  grid: {rows: 2, columns: 2, pattern: 'independent'},
  width: 500,
  height: 400
})

in 9f2fad1, we disallowed matches and scaleanchor constraints with different target axes. Perhaps I misinterpreted

I guess it may be clearer that way, as it means “range matches AND scale matches”... just seems weird as the only allowed values of scaleanchor would be ’x’ or nothing.

from your #1549 (comment)

@etpinard

This comment was marked as outdated.

Copy link
Member Author

etpinard commented Feb 5, 2019

@nicolaskruchten we don't currently match auto-binning for histogram2d traces on the same subplot. At present, I think it would feel weird to add a way to make them match across subplots, but have no way to make them match on the same subplot. I'm thinking we need another histogram2d attribute (e.g. bingroup). Could we split this item into separate issue?

Edit: we could also add "shared" (x|y)bins objects in the layout similar to how "shared" color scales/axes will be added shortly.

@nicolaskruchten

This comment was marked as outdated.

Copy link
Member

nicolaskruchten commented Feb 5, 2019

Could we split this item into separate issue?

Yep!

@etpinard

This comment has been minimized.

Copy link
Member Author

etpinard commented Feb 11, 2019

@nicolaskruchten @alexcjohnson would it a big deal if we disallowed simultaneous use of matches and scaleanchor on the same set of axes for this first iteration?

@nicolaskruchten

This comment has been minimized.

Copy link
Member

nicolaskruchten commented Feb 11, 2019

In principle that's fine... I would assume that matched totally supersedes scaleanchor anyway, no?

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

alexcjohnson commented Feb 11, 2019

In principle you can have both, if you have constrain: 'domain' too. Anyway, I'm fine with omitting it from v1 as long as it's clear we can add it later. Let's have matches win and drop scaleanchor when necessary until they're supported together.

@etpinard

This comment has been minimized.

Copy link
Member Author

etpinard commented Feb 11, 2019

Great. Thanks! Sorry for quitting on this problem, but I'll like to get going on sunburst ASAP.

drop *scaleanchor* constraints for axes under *matches* constraint
... for now, until we found a way to apply domain scaleanchor constraints
    on matching axes (which is theoretically possible).
@etpinard

This comment has been minimized.

Copy link
Member Author

etpinard commented Feb 12, 2019

. Let's have matches win and drop scaleanchor when necessary until they're supported together.

Done in 91431ec

@plotly/plotly_js this PR is now ready to review 👀 As this PR touches some fairly important cartesian subplot concepts, I'll like both @antoinerg and @archmoj to review this thing. Thanks!

@nicolaskruchten

This comment has been minimized.

Copy link
Member

nicolaskruchten commented Feb 12, 2019

Just to make sure I get the scaleanchor limitations... it means it’s not possible to have two plots with matched axes that are both constrained to be physically square?

@etpinard

This comment has been minimized.

Copy link
Member Author

etpinard commented Feb 12, 2019

it means it’s not possible to have two plots with matched axes that are both constrained to be physically square?

that's correct. (sorry 😢 )

@nicolaskruchten

This comment has been minimized.

Copy link
Member

nicolaskruchten commented Feb 18, 2019

that's correct. (sorry 😢 )

I think that's totally reasonable to start with!

@etpinard

This comment has been minimized.

Copy link
Member Author

etpinard commented Feb 18, 2019

Ping @archmoj @antoinerg can one of you please review this thing today? Thank you.

@archmoj archmoj self-requested a review Feb 18, 2019

@archmoj
Copy link
Collaborator

archmoj left a comment

@etpinard Excellent!
Please find my few comments below.

Show resolved Hide resolved src/plots/cartesian/set_convert.js
Show resolved Hide resolved src/plots/cartesian/dragbox.js Outdated
Show resolved Hide resolved src/plots/cartesian/dragbox.js Outdated
Show resolved Hide resolved src/plot_api/subroutines.js
@antoinerg

This comment has been minimized.

Copy link
Collaborator

antoinerg commented on src/plots/cartesian/layout_attributes.js in 772efe5 Feb 18, 2019

Did you forget the word axes? matching axes must have the same type

This comment has been minimized.

Copy link
Member Author

etpinard replied Feb 18, 2019

@archmoj

This comment has been minimized.

Copy link
Collaborator

archmoj commented Feb 18, 2019

@antoinerg it is a 💃 from my side, if you have no more comments?

@antoinerg

This comment has been minimized.

Copy link
Collaborator

antoinerg commented Feb 18, 2019

It is also a 💃 from me!

@etpinard eventually I'd be curious to know what hurdles you ran into and how long or hard it woud be to support constrained axes.

Anyway, nice PR 💪

@etpinard

This comment has been minimized.

Copy link
Member Author

etpinard commented Feb 18, 2019

I'd be curious to know what hurdles you ran into and how long or hard it woud be to support constrained axes.

That's in -> #3539

@etpinard etpinard merged commit 25fa0c2 into master Feb 18, 2019

8 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: publish Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-image2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details

@etpinard etpinard deleted the matching-axes-final branch Feb 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.