Skip to content

DOC: Fixup to AnchoredArtist examples in the gallery #11093

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

Merged
merged 1 commit into from
Apr 28, 2018

Conversation

madphysicist
Copy link
Contributor

Response to issue #11092. Instead of deleting the user gallery example called anchored_bbox01, I made it a non-duplicate. It did not make sense to remove it entirely given that there are perfectly reasonable anchored_bbox examples up to 04. I did remove the clearly obsolete misc/anchored_artists.html and renamed axes_grid1/simple_anchored_artists.html to axes_grid1/anchored_artists.html. Hopefully the changes I made to the comments and docstrings in the example came out as an improvement.

@ImportanceOfBeingErnest
Copy link
Member

See comment on original issue #11092. In short: they are non-duplicates. So at most, I'd recommend adding explanations above the respective examples providing the context and link to the other one(s).

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Blocking as per @ImportanceOfBeingErnest comments. Suggest modifying this PR or opening a new one...

at = AnchoredText("Figure 1a",
prop=dict(size=15), frameon=True, loc=2)
# loc=8 is equivalent to loc='lower center'
at = AnchoredText("Here is some\nsample text",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ImportanceOfBeingErnest @jklymak Would this be considered an acceptable change?

  1. Modified the string to have a newline, because that (A) makes it less redundant, and (B) shows off the capabilities a little better
  2. Changed (and documented) the loc parameter.

@madphysicist madphysicist force-pushed the anchored-artist-gallery branch from 35f70d8 to 5955aa2 Compare April 20, 2018 18:37
@@ -56,13 +74,12 @@ def draw_sizebar(ax):
ax.add_artist(asb)


if 1:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why that was here in the first place...

@@ -18,27 +24,34 @@ def __init__(self, s, loc, pad=0.4, borderpad=0.5,
child=self.txt, prop=prop, frameon=frameon)


class AnchoredSizeBar(AnchoredOffsetbox):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing got deleted here, just moved around so the functions line up with the other file


if __name__ == "__main__":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, OK. I did delete this line, and made the same set of small functions as in the similar file to highlight the parallels.


plt.draw()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I got rid of this line. It doesn't do anything useful if you call show next.

@madphysicist madphysicist force-pushed the anchored-artist-gallery branch from 60619db to 2bf5414 Compare April 20, 2018 18:42
@madphysicist
Copy link
Contributor Author

@ImportanceOfBeingErnest @jklymak. I think you will find my changes much more acceptable now. I moved the code around in the non-toolkit file to line up better with the yes-toolkit one. Besides that, I added a bunch of comments to clarify why there are two files there to begin with. I've documented all the other changes I made through review comments here, all of which I think are legitimate cleanup.

@madphysicist madphysicist force-pushed the anchored-artist-gallery branch from 2bf5414 to ef7c672 Compare April 20, 2018 18:54
@ImportanceOfBeingErnest
Copy link
Member

In the simple Anchored Box01 example, I would not change the text actually. This example is meant to be the simplest case for an anchored text (as opposed to a text positionned by coordinates). But you can get rid of the loc=2 and replace it with "upper left". As said it would be good if the code shown in the example and the user guide are equal.

at = AnchoredText("Figure 1a",
                  prop=dict(size=15),
                  frameon=True,
                  loc="upper left")

Maybe instead you can use the misc_anchored_artists example to display a two-line text, if that is important for you?

As said, I think there should be links between the files. E.g. to link from the misc_anchored_artists example to
to the axes_grid1_simple_anchored_artists you'd use

:ref:`sphx_glr_gallery_axes_grid1_simple_anchored_artists.py`

@madphysicist
Copy link
Contributor Author

What exactly would be the purpose of :ref:`sphx_glr_gallery_axes_grid1_simple_anchored_artists.py`? The text would just appear in a comment, or am I missing something?

@ImportanceOfBeingErnest
Copy link
Member

The purpose would be that one can click on the link and land on the example page. Did you try that? Is it not working?

@madphysicist
Copy link
Contributor Author

@ImportanceOfBeingErnest I actually don't know how to build the docs locally. Will figure it out and let you know.

@ImportanceOfBeingErnest
Copy link
Member

Building the docs can be a little bit cumbersome the first time you try it, especially on windows. Of course it's ideal to test locally first before committing anything, but you may also rely on the CI tests here on the server for building the docs. It might be a bit overkill going through all the hassle for testing if a single link it working correctly.

@madphysicist
Copy link
Contributor Author

I'm doing this on a Linux box that's not doing much else at the moment. It sounds like the kind of thing I'd want to try once for fun just to get an idea, so if you have a pointer to some instructions, I'd be happy to do it.

@jklymak
Copy link
Member

jklymak commented Apr 23, 2018

@ImportanceOfBeingErnest
Copy link
Member

In general, the instructions at writing documentation apply. In addition make sure not to use Sphinx 1.7.3. Have an up to date latex available, including xetex. I think that's it for linux. (You never know what else can go wrong until you try it.)

@jklymak jklymak dismissed their stale review April 23, 2018 17:32

Stale review

@madphysicist
Copy link
Contributor Author

@ImportanceOfBeingErnest Tried it, ran into the issue of no such package matplotlib. I would prefer to build the docs in place without installing mpl into my current environment. I suspect that this can be done by setting up the PYTHONPATH to point to my source folder, but I was hoping there is a more "appoved" solution.

@ImportanceOfBeingErnest
Copy link
Member

Mhh not sure if I understand this. Did you build matplotlib in a source folder and then installed it in a python environment? In that case inside that environment you should be able to do import matplotlib - that's the whole point of it, right? Otherwise you couldn't even test any of the changes to the code.

@madphysicist
Copy link
Contributor Author

@ImportanceOfBeingErnest. I was basically looking for python setup.py develop, or more optimally pip install -e .. My lack of knowledge of the build system is apalling. I am used to numpy's runtests.py, which builds everything in place and does not properly install it as far as I am aware.

No files were delete, instead, a clarifying comment was added to
indicate why there are nearly duplicate files sitting around.

[ci skip] [skip ci]
@madphysicist madphysicist force-pushed the anchored-artist-gallery branch from 253bc73 to 6f7388c Compare April 24, 2018 18:39
@madphysicist
Copy link
Contributor Author

I've made all the requested changes. I did not understand how the comments in the examples were being rendered because most of the examples I have seen don't have proper docstrings. Thanks for bearing with me on this one. I've learned a lot about building and installing matplotlib, as well as how to work with the docs.

Copy link
Member

@ImportanceOfBeingErnest ImportanceOfBeingErnest left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@jklymak
Copy link
Member

jklymak commented Apr 28, 2018

Thanks for the effort on this @madphysicist

@jklymak jklymak added this to the v2.2-doc milestone Apr 28, 2018
@jklymak jklymak merged commit cc4d9ce into matplotlib:master Apr 28, 2018
@lumberbot-app
Copy link

lumberbot-app bot commented Apr 28, 2018

There seem to be a conflict, please backport manually

@jklymak jklymak modified the milestones: v2.2-doc, v3.0 Apr 28, 2018
@madphysicist madphysicist deleted the anchored-artist-gallery branch April 28, 2018 21:25
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.

4 participants