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 Updated parameters to numpy format #7036

Merged
merged 10 commits into from
Jul 30, 2017

Conversation

djarecka
Copy link
Contributor

@djarecka djarecka commented Sep 5, 2016

WIP: Updating documentation of the pcolor function in _axes.py.
Updates to Parameters
TODO: Returns, Examples, etc.

@NelleV

@QuLogic
Copy link
Member

QuLogic commented Sep 6, 2016

Your commit is currently attributed to "djarecka". It is your choice whether to use a real name or not, of course, but I'm making a note of it just in case.

@NelleV
Copy link
Member

NelleV commented Sep 6, 2016

Note for reviewers: this is still work in progress.

Y coordinates of the colored quadrilaterals.
cmap : `~matplotlib.colors.Colormap`, optional, default: None
If `None`, default to rc settings.
norm :`matplotlib.colors.Normalize`, optional, default: None
Copy link
Member

Choose a reason for hiding this comment

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

There is trailing whitespace at the end of this line. Can you please remove it?

@QuLogic
Copy link
Member

QuLogic commented Sep 6, 2016

Your last two commits do not have a name set and are currently attributed to "Your Name you@example.com", which is probably not what you want.

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Sep 24, 2016
@NelleV
Copy link
Member

NelleV commented Oct 17, 2016

Hi @djarecka
I remember you expressed interest in pursuing the work for this PR. Are you still interested?
Thanks,

@djarecka
Copy link
Contributor Author

Hi @NelleV
I'm sorry, it just at the end I still had some problems with my matplotlib and couldn't see my changes in the documentation. Later I was traveling and completely forgot. I'll try to install sphinx and build documentation on my new computer or finish the PR without it. Will do it this week!

@QuLogic
Copy link
Member

QuLogic commented Oct 23, 2016

Are you familiar with rebasing and willing to fix the name on the previous two commits? If not, just tick the checkbox on the right column for "Allow edits from maintainers" and we can do that for you.

@djarecka
Copy link
Contributor Author

@QuLogic , never done rebasing, but let me read and try. However, I'll finish my changes in documentation first.

Copy link
Member

@NelleV NelleV left a comment

Choose a reason for hiding this comment

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

Thanks so much for continuing this work!

Overall, it looks good. I've underline two small problems that prevents the documentation from building properly. I think it should work fine once you fix those (thought we will have to check).

-------
kwargs : `~matplotlib.collections.PolyCollection` properties:

%(PolyCollection)s
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this line? We have decided to move away from docstring interpolation. It should also fix the build problem.


%(PolyCollection)s

The default `antialiaseds` is False if the default
Copy link
Member

Choose a reason for hiding this comment

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

We also need to have that part as a "numpydoc" argument style:

antialiaseds : bool, optional, default: False
    blah blah blah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @NelleV , thanks for suggestions. I've been testing this doc a bit on a private branch, but had problems with understanding the problem. Did you mean removing the line 5259 with %(PolyCollection)s. I did it, but didn't help and I saw it in other parts of the file. Will try to test it more during weekend, but if you have other suggestion, I'm happy to try.

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Let us know if you want to do the rebase yourself to correct the names.

@djarecka
Copy link
Contributor Author

@QuLogic I guess my bigger problem is that I'm still getting errors... I tested a few things today on a different branch, but didn't learn anything interesting. If you know what is wrong and how to fix it, that would be great.

@NelleV NelleV modified the milestones: 2.1 (next point release), 2.0.1 (next bug fix release) Dec 17, 2016
details, see the :ref:`Grid Orientation
<axes-pcolor-grid-orientation>` section below.
pcolor can be very slow for large arrays; consider
using the similar but much faster
Copy link
Member

Choose a reason for hiding this comment

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

The problem is that this line is indented and it confuses sphinx; aligning it with the previous line should fix it.

An array of color values.

X, Y : array_like, optional
If given, specify the (x, y) coordinates of
Copy link
Member

Choose a reason for hiding this comment

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

Please stick to 4-space indent.

@dstansby
Copy link
Member

Hi @djarecka, I have fixed the last couple of things that should make the doc build work - it took me a while to fix everything, so I'm not surprised you had problems. Hopefully if the doc build passes a couple of us can review this and get it merged - thanks for the PR!

@djarecka
Copy link
Contributor Author

@dstansby - thank you for fixing!

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

There are some rendering issues, noted below. If you rebase, please fix the name on the commit.

An :class:`matplotlib.colors.Normalize` instance is used
to scale luminance data to 0,1. If *None*, defaults to
:func:`normalize`.
norm :`matplotlib.colors.Normalize`, optional, default: None
Copy link
Member

Choose a reason for hiding this comment

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

Space after colon.

Other Parameters
----------------
antialiaseds : bool, optional, default: False
The default `antialiaseds` is False if the default
Copy link
Member

Choose a reason for hiding this comment

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

Double-backticks (can't reference parameters)

----------------
antialiaseds : bool, optional, default: False
The default `antialiaseds` is False if the default
`edgecolors`="none" is used. This eliminates artificial lines
Copy link
Member

Choose a reason for hiding this comment

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

Using a backtick in the middle of a word doesn't close the reference. In any case, should be double-backticks because you can't reference parameters.

Stroking the edges may be preferred if `alpha` is 1, but
will cause artifacts otherwise.

kwargs : `~matplotlib.collections.PolyCollection`
Copy link
Member

Choose a reason for hiding this comment

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

This is not quite correct; any remaining **kwargs are sent to PolyCollection. kwargs is not expected to be a PolyCollection type object.

@QuLogic
Copy link
Member

QuLogic commented Jul 29, 2017

I fixed the formatting stuff and updated the commit author; this can probably be squash-merged.

@QuLogic QuLogic changed the title [WIP] DOC Updated parameters to numpy format DOC Updated parameters to numpy format Jul 29, 2017
@QuLogic QuLogic merged commit 189cb98 into matplotlib:master Jul 30, 2017
@dstansby dstansby removed their assignment Jul 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants