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

DOC: Tiny fixes, and possible overhaul, of the two scales example in the gallery #10320

Merged
merged 5 commits into from Jan 25, 2018

Conversation

Projects
None yet
5 participants
@afvincent
Copy link
Contributor

commented Jan 25, 2018

PR Summary

Two-fold PR about the gallery example that shows how to use ax.twinx to display two scales on the same plot:

  1. 367647e fixes a few typos and introduces a call to fig.tight_layout to prevent the y-labels from being clipped
  2. fdb5900 is a significant style overhaul that:
    • compresses the docstring of the (main) helper function;
    • gets rid of the minor helper function used to color the y-tick labels, to simply include the relevant code into the main helper function (this avoids passing the desired colors a second time);
    • now colors the y-labels as well;
    • sets up the axes labels in a place that actually makes sense (beforehand, those labels were hard-coded into the main helper function, independently of whatever data1 and data2 could be...).

I agree that the value of the 2nd commit may be subjective (although I personally think that the helper function is now quite more logical, and more suited to reusing outside of the gallery) and I would understand if it were discarded simply because of that. However, if it was the case, the 1st commit fixes minor issues that, I think, are objective enough to still be this commit.

Here is what the example looks like with both commits:
with_both_commits

PR Checklist

  • Code is PEP 8 compliant: I think so, CI will decide
  • Documentation is sphinx and numpydoc compliant: AFAIU, it should be

NB: milestoned for 2.2 as it is only a small piece of documentation. Feel free to re-milestone for 3.0 if needed.

@afvincent afvincent added this to the v2.2 milestone Jan 25, 2018

@timhoffm

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

I would get rid of the main helper function as well. It just saves duplicating the four lines of plot code for the second axes, at the cost of a lot of docstring and loop over a parameter set. As such it‘s distracting from the main point twinx.

Also, you wouldn‘t likely see such a function in real life. It’s too specific, except if you would do a lot of these plots, which is beyond the scope of this example.

@afvincent

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

FWIW I had tried to keep some of the previous functional approach as I know that @tacaswell likes to promote it (IIRC). Anyway, a straight to the point example is fine with me too, so here it is.

(About the real-life plausibility, well I used such helper functions at my day-job ^^...)

BTW: thanks @timhoffm for the remark :).

@dopplershift
Copy link
Contributor

left a comment

While I'm all for more functions, the original example here bordered on being a counter example on the use of functions. Big 👍 on the new version.

data1 = np.exp(t)
data2 = np.sin(2 * np.pi * t)

# Create twin axes and plot the mock data onto them

This comment has been minimized.

Copy link
@QuLogic

QuLogic Jan 25, 2018

Member

This comment is kind of "small" for its contents. That is, it describes the next two or three blocks, but is just a regular comment. I feel like it should be broken up to each block, or made a bit more distinct visually.

@afvincent

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

@QuLogic Is it better with the comments like in ce43564?

@timhoffm

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

Wouldn't it be more instructive for twinning, to plainly write everything out? E.g.

fig, ax1 = plt.subplots()
		
ax1.set_xlabel('time (s)')
ax1.set_ylabel('exp', color='red')
ax1.plot(t, data1, color='red')
ax1.yaxis.set_tick_params(labelcolor='red')

ax2 = ax1.twinx()  # instantiate a second axes that shares the same x-axis

ax2.set_ylabel('sin', color='blue') # NB: we already took care of the x-label with ax1
ax2.plot(t, data2, color='blue')
ax2.yaxis.set_tick_params(labelcolor='blue')

fig.tight_layout()  # otherwise the right y-label is slightly clipped
plt.show()
@dopplershift

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

I like @timhoffm 's version better.

@afvincent

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

I am not sure that I totally agree but I am too biased anyway 😈, so here is the unrolled version adapted from @timhoffm's suggestion.

Being there, slight update of the color for something more in phase with Matplotlib 2+ :):
overhaul_of_the_overhaul

@jklymak jklymak merged commit a4c803a into matplotlib:master Jan 25, 2018

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci: docs-python27 Your tests passed on CircleCI!
Details
ci/circleci: docs-python35 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
lgtm analysis: Python No alert changes
Details
@afvincent

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

Thanks all :)!

@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018

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.