SG for toolkits #8573
SG for toolkits #8573
Conversation
ead77c1 to
71d6118
Compare
|
pretty sure travis error is not related to this PR |
|
What is ntbk? You've also accidentally added a few IPython notebook checkpoint files which, being rather large, should be excised from the repo completely. |
|
shit - my bad, those are just my own personal temporary notebooks that I use for debugging matlab. removed in latest push |
|
I was wondering why there were so many changed files in this PR :P |
|
not sure what this error is about - it seems to be about pandas datetimes or something |
|
I think the pandas issue was fixed in master? Could you rebase and force-push, please? |
|
ok just pushed a rebase |
doc/api/toolkits/axes_grid.rst
Outdated
| :toctree: ../_as_gen | ||
| :template: automodule.rst | ||
|
|
||
| axes_grid |
There was a problem hiding this comment.
A decision needs to be made about what to do with axes_grid. We have been putting these warnings up for its use (telling people to use axes_grid1 and axisartist, but it is all very confusing to end-users.
There was a problem hiding this comment.
My opinion is to get rid of axes_grid in the documentation if you want people to stop using it and axes_grid1 has been around for a while. I can say at least from personal experience it's super confusing what the difference between the two is...
There was a problem hiding this comment.
I can remove this in the api page in this PR and include a link to the old documentation in previous 2.x iterations in the axes_grid1 page. WDYT?
There was a problem hiding this comment.
latest push has this in there
doc/api/toolkits/mplot3d.rst
Outdated
| :toctree: ../_as_gen | ||
| :template: autosummary.rst | ||
|
|
||
| axes3d.Axes |
There was a problem hiding this comment.
This is just an import of the 2d base axes class. Take it out
doc/api/toolkits/mplot3d.rst
Outdated
|
|
||
| axes3d.Axes | ||
| axes3d.Axes3D | ||
| axes3d.Bbox |
There was a problem hiding this comment.
Just an import of the 2d bbox class. Take it out.
doc/api/toolkits/mplot3d.rst
Outdated
| axes3d.Bbox | ||
| axes3d.LightSource | ||
| axes3d.Normalize | ||
| axes3d.Triangulation |
There was a problem hiding this comment.
These three are also just imports from the core matplotlib. The only thing from this list to be kept is Axes3D.
doc/api/toolkits/mplot3d.rst
Outdated
| axis3d.ZAxis | ||
| axis3d.get_flip_min_max | ||
| axis3d.move_from_center | ||
| axis3d.tick_update_position |
There was a problem hiding this comment.
Again, I would keep only axis3d.Axis. Everything else is either uninformative, or not intended for the end-users.
| art3d.poly_collection_2d_to_3d | ||
| art3d.rotate_axes | ||
| art3d.text_2d_to_3d | ||
| art3d.zalpha |
There was a problem hiding this comment.
The top 8 items (the 3d artist classes) should definitely be kept. The rest of these... I would think they would be clutter. At the very least, they should be separated out, as they are more utility-like than anything else. I do see that they are included in the current documentation, though.
There was a problem hiding this comment.
I separated them out into a separate set of utility functions
| e.g., x2, y2 = trans(x1, y1) | ||
| should return two target coordinates.:: | ||
|
|
||
| e.g., x2, y2 = trans(x1, y1) |
There was a problem hiding this comment.
most of these I'm just converting the rST so that it renders as code properly...I don't think I have the expertise to fix bugs in the actual implementations etc
There was a problem hiding this comment.
That's not what I meant. I meant that this isn't supposed to be a codeblock. "e.g.," isn't code.
There was a problem hiding this comment.
gah crap you're right - I'll just make it "e.g., x2, y2 = trans(x1, y1)" sounds good?
There was a problem hiding this comment.
yup. that's what I think it should be.
tutorials/toolkits/mplot3d.py
Outdated
| The mplot3d Toolkit | ||
| =================== | ||
|
|
||
| Generating 3D plots the mplot3d toolkit. |
|
comments etc addressed I believe |
|
@WeatherGod LMK! Trying to clean up the open PRs I have now... |
|
ping! or somebody else wanna take a look? Maybe @story645 ? |
doc/api/toolkits/axes_grid.rst
Outdated
| --------- | ||
| .. note:: | ||
|
|
||
| There is an older version of the AxesGrid toolkit, simply called ``axes_grid`` instead of |
There was a problem hiding this comment.
I think this is all you need:
There is an older version of the AxesGrid toolkit, called
axes_grid
doc/api/toolkits/axes_grid.rst
Outdated
|
|
||
| There is an older version of the AxesGrid toolkit, simply called ``axes_grid`` instead of | ||
| ``axes_grid1``. the toolkit had a single namespace of axes_grid. This toolkit was broken | ||
| into the two modules below. For the documentation on the original ``axes_grid`` |
There was a problem hiding this comment.
For the documentation on
axes_grid
basically distinguish them just as axes_grid and `axes_grid1``
There was a problem hiding this comment.
Sounds good - but gosh, this is a confusing API choice :-P
There was a problem hiding this comment.
When I teach colleagues about making good design decisions early on, I always use this as an example of what can happen when hasty decisions are made.
doc/api/toolkits/mplot3d.rst
Outdated
| .. note:: | ||
| Historically, axis3d has suffered from having hard-coded constants | ||
| controlling the look and feel of the 3D plot. This precluded user | ||
| level adjustments such as label spacing, font colors and panel colors. |
There was a problem hiding this comment.
This precludes because the following sentence indicates that these hard-coded constants are still part of the module.
There was a problem hiding this comment.
No, the hard-coded constants are gone (at least, for the most part).
There was a problem hiding this comment.
Yeah, it's the "for the most part" that's unclear here. What do the current hardcoded constants affect and does the user need to know about it.
There was a problem hiding this comment.
The problem is that the 3d axis objects aren't done the same as any other axis objects. There are still some hard-coded constants, but they don't have anything to do with normal axis properties, so it is difficult to map those concepts back to what users would know.
The important thing is to remember the context. Pre v1.1.0, you couldn't configure anything in mplot3d. Users aren't precluded from configuring mplot3d any more.
There was a problem hiding this comment.
Ok, so what about this note do users need to know?
There was a problem hiding this comment.
The entire note is useful information to the end-user that wants to fully customize the mplot3d properties. The note has been there since v1.1.0. The kludge dictionary still exists, hence the note is still needed to document it.
I think we have spent enough time discussing this note. If you wish to eliminate this note, then please re-implement all of axis3d so that it would work like a normal axis object.
There was a problem hiding this comment.
If you wish to eliminate this note
I don't believe that @story645 wishes to eliminate the note, I think they just want to make sure the information is as clear as possible. For example, the most important bit of information lies at the very end of the note. I'd propose that we move it to the beginning, so something like:
"
See mpl_toolkits.mplot3d.axis3d._axinfo for a dictionary containing constants that may be modified for controlling the look and feel of mplot3d axes (e.g., label spacing, font colors and panel colors). Historically, axis3d has suffered from having hard-coded constants that precluded user adjustments, and this dictionary was implemented in version 1.1 as a stop-gap measure.
"
I think that pushes the most important information (the name _axinfo and what it does) to the front. WDYT @WeatherGod ?
There was a problem hiding this comment.
I'm happy to make that wording switch or to leave it the way it is...just throwing this out there since I'm already taking a pass through the code. LMK what you'd prefer!
There was a problem hiding this comment.
I am fine with that proposed change.
There was a problem hiding this comment.
@WeatherGod I'm sorry, I should have phrased my comment better. Like @choldgraf said, I found the note really confusing as to what the user could and couldn't now modify. I really like @choldgraf's new wording.
| @@ -3,9 +3,11 @@ | |||
| Matplotlib axisartist Toolkit | |||
| ============================= | |||
|
|
|||
| .. toctree:: | |||
| :maxdepth: 2 | |||
| The *axisartist* namespace includes a derived Axes implementation. The | |||
There was a problem hiding this comment.
link to the derived Axes implementation?
| .. toctree:: | ||
| :maxdepth: 2 | ||
| The *axisartist* namespace includes a derived Axes implementation. The | ||
| biggest difference is that the artists responsible to draw axis line, |
There was a problem hiding this comment.
for drawing the (is this the axis spine?), ticks, tick labels, and labels on an axis
tutorials/toolkits/axisartist.py
Outdated
| axisartist with ParasiteAxes | ||
| ---------------------------- | ||
|
|
||
| Most commands in the axes_grid1 toolkit can take a axes_class keyword |
tutorials/toolkits/axisartist.py
Outdated
| ---------------------------- | ||
|
|
||
| Most commands in the axes_grid1 toolkit can take a axes_class keyword | ||
| argument, and the commands creates an axes of the given class. For example, |
tutorials/toolkits/axisartist.py
Outdated
| host = host_subplot(111, axes_class=AA.Axes) | ||
|
|
||
|
|
||
| Here is an example that uses parasiteAxes. |
There was a problem hiding this comment.
two spaces between uses and parasiteAxes..and link to parasiteAxes?
There was a problem hiding this comment.
I'm not sure where the parasiteAxes class is...the example doesn't actually import it. Any idea?
tutorials/toolkits/axisartist.py
Outdated
| Curvilinear Grid | ||
| ---------------- | ||
|
|
||
| The motivation behind the AxisArtist module is to support curvilinear grid |
tutorials/toolkits/axisartist.py
Outdated
| Floating Axes | ||
| ------------- | ||
|
|
||
| This also support a Floating Axes whose outer axis are defined as |
There was a problem hiding this comment.
This [something] also supports
outer axis? (some singular plural weirdness here)
|
@story645 I'm happy to make these copy edits if you're happy to merge after I make them ;-) assuming that @WeatherGod is OK with the changes I made of course |
|
My approval is for the mplot3d side of things. I probably won't get around to reviewing the other toolkit's docs. |
|
OK @story645, I think most of these comments are addressed (except where I note clarifying questions) In the future, I am 100% fine with you just directly editing the PR with these changes. For small wording changes, type-os, etc, it's a waste of time for you to take a pass through and type out all of the changes as comments, and then to have me go back through and enter them all into the PR. Actually on that note, I'd be a fan of codifying this as official MPL documentation PR protocol. Comments are purely high-level stuff, and then just before merging there is a copy-editing pass that directly edits the PR before (squashing if necessary) merging. |
|
Totally agree with you on codifying, but not quite sure where...maybe here: http://matplotlib.org/devdocs/devel/contributing.html#contributing-pull-requests |
|
Also let me know if you're ready to merge. |
|
ok that looks good - feel free to merge @story645 (the last commit was just 2 lines of CSS so won't affect tests) |
|
actually it looks like there are some weird build errors - any idea what's going on w/ Travis @story645 ? |
|
Looks like it's a bug in sphinx, see comment from @dstansby in the gitter |
|
I don't have time for a PR at the moment, but forcing pip to install sphinx=1.5.6 in |
|
@dstansby don't you figure sphinx will release a patch pretty quickly? |
|
It's a sphinx-gallery issue, not a sphinx issue, so I'm not sure how quick a fix will be. |
|
I made a PR to fix it so hopefully not too long |
|
So should I merge or wait on the bug fix? |
|
What version of sphinx does MPL use for the website? If it's the latest then the error will pop up there too :-/ |
|
The matplotlib.org/devdocs is build automatically by Travis so that would fail too. The main webpage is build manually. I would exclude Sphinx <1.6.0 for now |
| {% for item in classes %} | ||
| {{ item }} | ||
| {% endfor %} | ||
| {% for item in classes %}{% if item not in ['zip', 'map', 'reduce'] %} |
There was a problem hiding this comment.
for some reason they were popping up in the docs so I hard-coded them in...I'm not really sure why they were doing that
doc/api/toolkits/axes_grid.rst
Outdated
| objects, and in the new version this toolkit was broken | ||
| into the two modules below. For the documentation on ``axes_grid``, | ||
| see the `previous version of the docs | ||
| <http://matplotlib.org/2.0.1/mpl_toolkits/axes_grid/index.html#toolkit-axesgrid-index>`_. |
doc/api/toolkits/mplot3d.rst
Outdated
| See :attr:`mpl_toolkits.mplot3d.axis3d._axinfo` for a dictionary containing | ||
| constants that may be modified for controlling the look and feel | ||
| of mplot3d axes (e.g., label spacing, font colors and panel colors). | ||
| Historically, axis3d has suffered from having hard-coded constants |
There was a problem hiding this comment.
I think this extra space is breaking the build.
|
small changes made - I see that we have a PR that downgrades sphinx to 1.5.x so I think we are OK. |
|
CI sure seems slow today. |
|
yahoo! thanks! |
This PR converts the toolkits section into a sphinx-gallery API. It also adds a
toolkitssection to the tutorials page and moves the manuals there.