| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Overall this seems a useful API though I suspect we haven’t thought through how it will be used. Allowing sharing to be toggled like this seems like it will be ripe for misunderstandings. Could use some examples of why you think this is useful
Sorry, something went wrong.
| """ | ||
| Share the x-axis with *other*. | ||
|
|
||
| This is equivalent to passing ``sharex=other`` when constructing the |
There was a problem hiding this comment.
This docstring could be more explicit and non circular. For instance the verb to share would indicate that self shares with other, whereas the exact opposite happens. Further passing sharex=other calls this function so I don’t think it’s very helpful to say that this function is the same as that and expect the user to ferret out the behaviour
Sorry, something went wrong.
There was a problem hiding this comment.
"self shares with other, whereas the exact opposite happens"
What do you mean by "opposite"? Isn't sharing symmetrical? (well perhaps not deep in the implementation details, but from the user PoV it is)
Sorry, something went wrong.
There was a problem hiding this comment.
I meant in everyday English the transitive verb “to share” means one person shares Something with another
Sorry, something went wrong.
There was a problem hiding this comment.
Can you propose an alternate wording here?
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, the main point is that saying ax.sharex is "Equivalent to passing sharex=other when constructing..." is a circular doc, because when you construct, you call ax.sharex. This function ideally would define what ax.sharex does explicitly.
Sorry, something went wrong.
There was a problem hiding this comment.
Right now the docs of sharex (as kwargs to the Axes constructor) are
sharex, sharey : `~.axes.Axes`, optional
The x or y `~.matplotlib.axis` is shared with the x or
y axis in the input `~.axes.Axes`.
which is not much better :/ I agree this can be improved, but is arguably a separate issue?
Sorry, something went wrong.
There was a problem hiding this comment.
The docstring defines what the function does for the user, and one of our main problems with "share" is that people don't know what it means and even in the history of the code base different definitions have popped up. Right now I'm not 100% sure what this PR changed and I think it should be defined clearly, in plain English, what calling this function will do.
Sorry, something went wrong.
There was a problem hiding this comment.
ax0 = (some axes) ax1 = fig.add_subplot(...) ax1.sharex(ax0)
is equivalent to
ax0 = (some axes) ax1 = fig.add_subplot(..., sharex=ax0)
Now it is true that what the latter does is not really well documented either, but the doc available for ax1.sharex(ax0) is, well, exactly the same.
Sorry, something went wrong.
|
I'm explicitly not allowing toggling, or changing who you share with, just going from "not-shared" to "shared". Indeed, allowing unsharing makes the problem much more complex. The use case hinted above is: A A A B X X X B X X X B X X X Each A plots the distribution (histogram) of variable a in conditions a1, a2, a3; each B plots the distribution of variable b in conditions b1, b2, b3, and each X plots the joint distribution (scatter plot). Here you want to sharex across rows and sharey across columns, except that the first row does not share y's and the first column does not share x's (let's say you don't want to normalize your histograms). |
Sorry, something went wrong.
|
I understand your example, but it would be helpful if you actually provided one in the PR. |
Sorry, something went wrong.
|
Do you mean one as comments in the code? (where?) in the commit message? |
Sorry, something went wrong.
|
I meant as an example in the example folder |
Sorry, something went wrong.
|
Sure, added a simple example. |
Sorry, something went wrong.
|
First thoughts on this:
|
Sorry, something went wrong.
|
I guess I'm +0.5 on sharex_with() and +0.25 on sharex_from() (both are better than sharex()), let's see what others think. What about adding something like
to the docstrings? (I don't really intend to support "later" calls with this PR anyways.) |
Sorry, something went wrong.
There was a problem hiding this comment.
There might be unintended consequences, but let's try it.
Approved, subject to rebase.
Sorry, something went wrong.
This intentionally does not allow unsharing or changing the shared axes, as there are bigger questions on the API there. The API is named `Axes.sharex()` to allow for a later `Axes.unsharex()`, though (`set_sharex()` would be fine, but `set_unsharex()`? or perhaps `set_sharex(None)`, though). An example use case is creating a grid with `subplots()`, but with custom sharing relationships between the subplots -- e.g., sharex/sharey across all, except the first row of axes which only shares x with their column and the first column which only shares y with their lines.
|
rebased |
Sorry, something went wrong.
|
We seem to be getting a systematic failure--not just on this PR--in the macosx build on Travis, in test_bbox_inches_tight_raster[pdf], an image comparison failure. |
Sorry, something went wrong.
|
macOS failure is #17065; doc failure is just spurious inability to download a Sphinx inventory. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This intentionally does not allow unsharing or changing the shared axes,
as there are bigger questions on the API there (#9923 and issues linked there).
The API is named Axes.sharex() to allow for a later Axes.unsharex(),
though (set_sharex() would be fine, but set_unsharex()? or perhaps
set_sharex(None), though). Happy to hear suggestions.
An example use case is creating a grid with subplots(), but with
custom sharing relationships between the subplots -- e.g., sharex/sharey
across all, except the first row of axes which only shares x with their
column and the first column which only shares y with their lines.
PR Summary
PR Checklist