Skip to content

add_shape, add_layout_image, add_annotation for multiple facets at once, add_hline and add_vline (+add_vrect/add_hrect) #2558

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

Closed
wants to merge 59 commits into from

Conversation

nicholas-esterer
Copy link
Contributor

@nicholas-esterer nicholas-esterer commented Jun 10, 2020

Closes #2141 and closes #2140

Making it possible to specify multiple rows and columns when using add_shape, add_layout_image or add_annotation.
This pull request is to share the work, so a lot is missing still.

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or
    modified existing tests.
  • For a new feature, I have added documentation examples in an existing or
    new tutorial notebook (please see the doc checklist as well).
  • I have added a CHANGELOG entry if fixing/changing/adding anything substantial.

Need more comprehensive tests for all the other add_* functions and for all the
possible combinations of row and col specification.
Simply pass a tuple for row and/or col where the second arg == 'all'.
(maybe this is overkill though, as a user could easily pass list(range(...))
instead)
@nicholas-esterer nicholas-esterer marked this pull request as draft June 10, 2020 20:07

# Get grid_ref if specific row or column requested
if row is not None:
grid_ref = self._validate_get_grid_ref()
refs = grid_ref[row - 1][col - 1]
# TODO It is assumed that grid_ref has an equal number of columns in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this can be not-true, you can have really odd layouts: https://plotly.com/python/subplots/#multiple-custom-sized-subplots

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey so I checked this out and it turns out even in this case grid_ref is still a rectangle, just some of the entries are None. So we could still easily iterate through the grid_ref using something like

for r in range(1,len(grid_ref)+1):
    for c in range(1,len(grid_ref[0])+1):
        sub_plot=fig.get_subplot(row=r,col=c)
        if sub_plot is not None:
            ....

We just check if it is not None before doing anything to it 🔧

@nicholas-esterer
Copy link
Contributor Author

I greatly refactored the old approach and added basically this one function (it calls some helpers which you can check out in the code):

BaseFigure._select_subplot_coordinates(self, rows, cols, product=False)
This function will allow add_shape, add_trace, ... to select subsets of the subplots using the syntax described in the issue (e.g., rows='all', col=[1,2] selects all the rows in columns 1 and 2). However I've abandoned the (list,'all') syntax in favour of an extra option product that if True will form the cartesian product of the row and col lists and if False will just zip them together to form the coordinates of the subplots to address.

The idea is to change add_shape, add_trace, ... etc. so that if they receive the "magic" row or col arguments, they call _select_subplot_coordinates to get the subplots to address, and then call the same function again but on all the coordinates.

@nicholas-esterer
Copy link
Contributor Author

_select_subplot_coordinates only has to be put in BaseFigure.add_trace and BaseFigure._add_annotation_like.

@nicholas-esterer
Copy link
Contributor Author

We have decided to not support the cartesian product mode for row and col, so after adding some documentation this PR can be reviewed for merging 🥳

for the add_trace and add_{shape,layout_image,annotation} Figure methods.
@nicholas-esterer
Copy link
Contributor Author

Hmm seeing now that the update_* functions also take row and col arguments. Need to see if this has already been taken care of...

@nicholas-esterer
Copy link
Contributor Author

Ok so the PR doesn't affect update_layout nor the select_* but for some reason the documentation is not updated for add_trace...

This makes it possible to replace a shape in the layout with one where its one
axis is fixed to the paper. This makes it possible to easily implement functions
like add_vline, which add a horizontal line to a plot.
Still need to write tests though.
@nicholas-esterer
Copy link
Contributor Author

For reviewers:
This PR allows specifying row='all' and or col='all for all the Figure methods that use the functions:
BaseFigure._add_annotation_like and BaseFigure.add_trace.
Then from this functionality, you can add vertical or horizontal lines or boxes that span the paper so that they remain fixed at the edges of a plot when you pan it.
The new functions are BaseFigure.add_{vline,hline,vrect,hrect}.
Most changes happened in packages/python/plotly/plotly/basedatatypes.py but some documentation updates went into packages/python/plotly/codegen/figure.py.

@@ -8,6 +8,8 @@
import warnings
from contextlib import contextmanager
from copy import deepcopy, copy
import itertools
import pdb
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably shouldn't be here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(the pdb part I mean)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

itertools yes, pdb no

@nicolaskruchten
Copy link
Contributor

I think reusing xref and yref would be preferrable to adding new kwargs, given that we also accept all the kwargs from the underlying add_shape, anything we add here cannot be used in the future in shape... We're already adding row,col and annotation and ideally we'd keep it to that minimum :)

Making sure that position is for any orientation of the shape.
They can be added using the annotation keyword argument:

fig.add_hline(y=1,annotation=go.layout.Annotation(text="example 1"))

or you can specify their parameters using the annotation_ prefixed
keywords

fig.add_hline(y=1,annotation_text="example 2")

There's also a quick way to specify some commonly encountered positions,
through the annotation_position argument

fig.add_vrect(y=1,annotation_text="example 3",
    annotation_position="inside bottom left")
Moved the tests for axis spanning shapes and their annotated versions to
test_autoshapes.
@nicholas-esterer
Copy link
Contributor Author

nicholas-esterer commented Oct 2, 2020

hline, vline, hrect, vrect, can be annotated. To see an example of what this looks like, you can run from the root of this repo:

VISUALIZE=1 python packages/python/plotly/plotly/tests/test_core/test_autoshapes/test_annotated_shapes.py

Note: probably commit 8a62ba7 has to get checked out before you can do this.

Made common file to share functions for all tests
Wrote spec of annotation shape tests.
See
packages/python/plotly/plotly/tests/test_core/test_autoshapes/test_annotated_shapes.py
for information on how to configure the test (it may need to be updated
from time to time).
@nicolaskruchten
Copy link
Contributor

Annotations look great!

Some comments:

  1. I think row/col should default to "all" ... I had a major head-scratching moment with a facetted figure with a blank in the bottom left!
  2. when nothing is provided related to the annotation (i.e. not annotation_<anything> or annotation=None) then no annotation should appear ... fig.add_hline(y=5) should not result in a "new text" annotation
  3. annotation_position should accept "top left" as well as "left top" i.e. order shouldn't matter here
  4. annotation_position should return error messages on invalid input i.e. "top bottom" should not be accepted and do nothing

…shapes

do not try and set the most recently added layout object's xref and yref
if we wanted to add it to a plot with one subplot/facet.
Updated regex in EnumeratedValidator.build_regex_replacement
Before Python3.5 it seems keys were not sorted by default (although
json.dumps says they always aren't by default, but this might be related
to some other serialization).
@nicholas-esterer
Copy link
Contributor Author

But if we say fig.add_vline(x=5,annotation_text="some text") then the annotation position defaults to top right?

@nicolaskruchten
Copy link
Contributor

That sounds good

@nicolaskruchten
Copy link
Contributor

how close are the docs to being done here? Need any help or guidance?

@nicholas-esterer
Copy link
Contributor Author

Just have to change them to reflect the behaviour where specifying no row or col puts the shapes on all subplots. And I have to document the annotation_* keywords. I was going to also put them as possible kwargs in the doc string of add_hline etc. so it shows up on calling errors.

@nicholas-esterer
Copy link
Contributor Author

Actually I was mistaken, it is sufficient to have the documentation appear when calling help(fig.add_hline) + the online docs?

@nicolaskruchten
Copy link
Contributor

No, we need to have docs written in the MD files :)

Also: the build output from doc/apidoc should not be committed here please... I'm surprised it's not already in the .gitignore but that stuff is built in CI :)

@nicolaskruchten
Copy link
Contributor

OK, so once we release plotly.js 1.57.0, i'll update master to that branch and you can prepare a new PR that branches off of master and includes only the changes you need to make for this PR... that'll make it a lot easier to properly review :)

@nicolaskruchten nicolaskruchten removed this from the v4.x milestone Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants