add_shape, add_layout_image, add_annotation for multiple facets at once

[nicolaskruchten]

Right now if I want to add the same shape (i.e. a horizontal line) to every facet in a PX figure, I have to loop through and do it myself. It seems like having a way of saying row="*" or something might be handy.

[emmanuelle]

row='all'? As a linux user I love the * but it's a bit too geeky for most users.

[nicolaskruchten]

Once we do the easy version of #2141 then this should apply to add_hline and add_vline as well.

[nicholas-esterer]

It looks like the change can just be added by iterating over rows and columns in packages/python/plotly/plotly/basedatatypes.py:BaseFigure._add_annotation_like because, although no copy of the new_obj argument is made, calling id on the objects added to self.layout[prop_plural] shows each is stored in unique memory. I assume this uniqueness is to be maintained in future versions?

[nicholas-esterer]

For the specification of multiple rows and columns, I propose the following options

row	col	The rows and columns addressed
"all"	"all"	Every combination of row and column (the cartesian product)
"all"	list	For each column specified, every row
"all"	int	For the column specified, every row
list	"all"	For each row specified, every column
list	list	If len(row)==len(col) The row and column pairs formed by calling zip on both lists, otherwise all combinations of specified rows and columns
list	int	The rows in the specified column
int	"all"	All the columns in the specified row
int	list	The specified columns in the specified row
int	int	The specified row and column

[nicolaskruchten]

Thanks for starting to flesh this out! One thing I'd like to think about is whether we bolt this on to e.g. add_shape or whether we add a new plural method add_shapes analogous to the add_trace/add_traces pair we have. I'm reluctant to add new methods to go.Figure because we have so many but we should at least consider it.

If we add these new plural methods, then we could consider row=None to mean row="all", say.

[nicholas-esterer]

I've also added the possibility of passing the tuple (list,'all') for rows or columns. In this case, all the combinations of rows and columns are used to look up the subplot, even if rows / columns are the same length.

[nicholas-esterer]

Finally, range(...) can now also be passed (it simply gets converted to a list), but maybe this is overkill (the change to support this is very small, though).

[nicholas-esterer]

Thanks for starting to flesh this out! One thing I'd like to think about is whether we bolt this on to e.g. add_shape or whether we add a new plural method add_shapes analogous to the add_trace/add_traces pair we have. I'm reluctant to add new methods to go.Figure because we have so many but we should at least consider it.

If we add these new plural methods, then we could consider row=None to mean row="all", say.

I've currently implemented in change in the BaseFigure._add_annotation_like of packages/python/plotly/plotly/basedatatypes.py. This way it should work for all the other go.Figure.add_* functions. I haven't looked, but would we win this "polymorphism" if we added a new method? Howver, I'm also happy to move it to its own function, if that's preferable.

[nicolaskruchten]

Oh I hadn't realized you were already implementing :) Would you mind opening up a draft PR so I can see how you're approaching this?

[nicolaskruchten]

Are you tackling #2141 in the same PR or are you starting with this issue and then doing that one?

[nicholas-esterer]

Seems some existing related tests to base this off of are in
packages/python/plotly/plotly/tests/test_core/test_update_objects/test_update_annotations.py:TestSelectForEachUpdateAnnotations.test_add_annotations

[nicholas-esterer]

No just 2140 for now
Yes I'll a submit a PR. All the work is happening in the branch issue-2140 which I've been pushing to origin.
Probably you'll find that things could be modularized a bit. I thought I would push and then reorganize after some suggestions :)

[nicholas-esterer]

Would it be unreasonable to add this functionality to all the add_* commands that take row and col arguments?

[nicholas-esterer]

Also I'm wondering if there could be a strategy to just test the _add_annotation_like function, rather than test add_shape, add_layout_image etc.
More precisely, I would like to specify a function _add_annotation_at_coords_like that takes a list of row,column pairs and puts objects at those subplots, rather than specifying row,col separately. That way testing the functionality is easier: we just make sure this function works (perhaps by testing on an empty list of row,column pairs, etc. to cover the edge cases).
Then once we know that works, we just need to write a test for the function that takes the arguments for row and col and make sure it gives the desired list of pairs (e.g., row=[1,2],col=[2,4,6] gives [(1,2),(1,4),(1,6),(2,2),(2,4),(2,6)]).
This will make writing the tests less tedious and make the tests automatically cover new cases. Furthermore, maybe this functionality could be extended to other functions, like add_trace.

[nicolaskruchten]

Would it be unreasonable to add this functionality to all the add_* commands that take row and col arguments?

I think there's an important difference between traces and layout objects like annotations, shapes, xaxes etc: the former is "data" and if you want to add more than one you should do so explicitly, whereas the latter is more the frame of the data, and so it makes sense to want to add them in batch. So my gut feeling is that we should not bolt this onto add_trace (and we already have add_traces as noted).

That said, we could probably get some benefit from adding this functionality to update_* methods that support row and call e.g. update_xaxes, and my gut feeling is that it would be useful and appropriate to add this to update_traces as well. Also possibly the for_each_* methods.

[nicolaskruchten]

Re a testing strategy, I would support a layered strategy where we systematically test the inner logic (_add_annotation_like), and then we have a few spot-check tests for the downstream logic (add_shape).

[nicolaskruchten]

One nice thing to do in tests is to use https://docs.pytest.org/en/stable/parametrize.html to condense tests a bit. I used this a lot recently in PX tests like https://github.com/plotly/plotly.py/blob/master/packages/python/plotly/plotly/tests/test_core/test_px/test_px_wide.py#L48

[nicholas-esterer]

So now that Plotly.js will have the '[xy][2-9][0-9]* domain' axis ref syntax, how do we support adding such shapes to a specified row and column pair? Because currently if the xref and yref are specified for the shape they are overwritten with the axes corresponding to the row and column requested. So in this case do we let users just pass xref='domain' say? I'm not familiar enough with the current API to know if that would break a lot of stuff :-O

[nicholas-esterer]

Perhaps to please users that mistakenly put 'x domain' say and those that know that in fact you only need 'domain' we just check to see if the xref contains 'domain' (WLOG for yref) and if it does, make the shape reference domain on x+number-determined-by-row-col.

[nicolaskruchten]

So with the open PR, fig.add_trace(..., row="all", col="all") is accepted and does add the trace to all facets. I've sort of softened my opposition to this since my comments above #2140 (comment) but I think that if we do accept this we would want to do some extra magic on the legend...

can we block this from working with .add_trace() for now?

update: I think this .data[-1].update() trick is actually OK here. Let's leave it as-is :)

[nicholas-esterer]

note: seems add_{h,v}line doesn't work with multicategorical axes

[nicolaskruchten]

Do you mean that .add_hline(y=...) doesn't work when the x or y axis is multicat? or both?

[nicholas-esterer]

OK after looking into it a bit more, it looks like the problem is when you specify the coordinate for hline or vline that is the same as a 'multicategory' axis, the coordinate seems to default to half of the size of the axis. For example, here specifying a coordinate on a 'category' axis works, but not on 'multicategory'

import plotly.graph_objects as go
from plotly import subplots

fig=subplots.make_subplots(1,2)

fig.add_trace(go.Bar(x=['dog','cat','mouse'],y=[5,1,2]),row=1,col=1)
fig.update_xaxes(type='category',row=1,col=1)

fig.add_trace(go.Bar(
    x=[["A","A","B","B"],["flower","water","rock","grass"]],
    y=[5,1,2,3]
),row=1,col=2)
fig.update_xaxes(row=1,col=2,type='multicategory')

fig.add_vline(x='cat',row=1,col=1)
fig.add_vline(x='mouse',row=1,col=1)
fig.add_vline(x='dog',row=1,col=1)
fig.add_vline(x='water',row=1,col=2)
fig.add_vline(x='rock',row=1,col=2)
fig.add_hline(y=3,row=1,col=2)

fig.show()

[nicholas-esterer]

[nicholas-esterer]

But this bug seems to be from the JS side and it is also in master, so I don't think it's due to this PR (although I could try and fix it).

[nicolaskruchten]

So you can't just say "water" for multicategory, you'll have to say ["A", "water"] I believe.

[nicolaskruchten]

or maybe [["A"],["water"]] or something?

either way, so long as this works on numeric/date axes when the opposite axis is multi-cat then I'm happy (the hlines in this case)

[nicholas-esterer]

Ah ok nevermind! It works when you do fig.add_vline(x=['B','rock'],row=1,col=2) etc. 👍