Improve docstrings for visual interfaces and fix hline function #450
Improve docstrings for visual interfaces and fix hline function #450aatifjunaid wants to merge 2 commits intoarviz-devs:mainfrom
Conversation
OriolAbril
left a comment
OriolAbril
left a comment
There was a problem hiding this comment.
This is looking good thanks. I still have to go over all the docstrings to double check what they say is correct, so some small tweaks might be needed still later on. But before that after a quick skim, the main lacking thing is in **artist_kws. They should always indicate to what backend function things are forwarded too.
|
Is it better now? |
OriolAbril
left a comment
OriolAbril
left a comment
There was a problem hiding this comment.
I have stopped at text. I have no idea how you came up with the references for artist_kws and where they are forwarded too but please review them. The backend functions are very short so in the majority of cases it is straightforward to see what backend method/function gets called.
|
|
||
| * matplotlib -> :meth:`~matplotlib.figure.Figure.suptitle` | ||
| * plotly -> :meth:`~plotly.graph_objects.Figure.update_layout` (``title=...``) | ||
| * bokeh -> :attr:`~bokeh.plotting.figure.Figure.title` |
There was a problem hiding this comment.
the bokeh reference is incorrect. The function does something completely different, and whatever is linked here can never be an attribute, it is something callable because we forward keyword arguments to it when calling it.
| target : PlotObject | ||
| The backend object representing a :term:`plot` where this :term:`visual` | ||
| should be added. | ||
| bottom : float, default 0 |
There was a problem hiding this comment.
unless it is something that can't be an integer we try to use scalar instead.
Also, in this particular case I am quite sure arrays are also valid so I'd update it to:
| bottom : float, default 0 | |
| bottom : scalar or ndarray, default 0 |
| * matplotlib -> :meth:`~matplotlib.axes.Axes.plot` | ||
| * plotly -> :class:`~plotly.graph_objects.Scatter` with (``mode="lines"``) | ||
| * bokeh -> :meth:`~bokeh.plotting.figure.line` |
There was a problem hiding this comment.
both matplotlib and bokeh references are wrong here
| step_mode : any, optional | ||
| Defines how the step transitions are drawn (e.g. ``pre``, ``post``, ``mid``). | ||
| Interpretation depends on the plotting backend. |
There was a problem hiding this comment.
this one is not any, it should be string, in fact one of 3 possible strings only. Therefore:
| step_mode : any, optional | |
| Defines how the step transitions are drawn (e.g. ``pre``, ``post``, ``mid``). | |
| Interpretation depends on the plotting backend. | |
| step_mode : {"before", "center", "after"}, optional | |
| Defines how the step transitions are drawn. |
| Passed to the backend plotting function of the respective backend: | ||
|
|
||
| * matplotlib -> :meth:`~matplotlib.axes.Axes.step` | ||
| * plotly -> :class:`~plotly.graph_objects.Scatter` (``line_shape="hv"`` or similar) |
There was a problem hiding this comment.
| * plotly -> :class:`~plotly.graph_objects.Scatter` (``line_shape="hv"`` or similar) | |
| * plotly -> :class:`~plotly.graph_objects.Scatter` with (``mode="lines"``) |
|
|
||
| Parameters | ||
| ---------- | ||
| x, y : float or array-like |
There was a problem hiding this comment.
here I am quite sure only scalars are valid and arrays or array-like objects would error out. If you can please test it, otherwise let me know and I'll double check when I get some time. In any case, if x and y can be arrays, then string should also be one
| vertical_align, horizontal_align : any, optional | ||
| Alignment properties of the text. | ||
| Interpretation depends on the plotting backend. |
There was a problem hiding this comment.
neither of the two is backend dependent, vertical_align can be top, middle or bottom. And horizontal_align can be left, center, right.
| * plotly -> :class:`~plotly.graph_objects.layout.Annotation` | ||
| * bokeh -> :class:`~bokeh.models.annotations.Label` |
There was a problem hiding this comment.
bokeh and plotly references are wrong.
|
Hi, sorry for the late response — I got caught up with deadlines. Thanks a lot for the detailed feedback. I’ll go through each of your comments carefully, verify them against the backend implementations, and update the PR accordingly. Really appreciate your patience. |
2 participants
"function": "vline"reference inhline.