-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
DOC: Fixup to AnchoredArtist examples in the gallery #11093
Conversation
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). |
There was a problem hiding this 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...
examples/userdemo/anchored_box01.py
Outdated
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", |
There was a problem hiding this comment.
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?
- Modified the string to have a newline, because that (A) makes it less redundant, and (B) shows off the capabilities a little better
- Changed (and documented) the
loc
parameter.
35f70d8
to
5955aa2
Compare
@@ -56,13 +74,12 @@ def draw_sizebar(ax): | |||
ax.add_artist(asb) | |||
|
|||
|
|||
if 1: |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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__": |
There was a problem hiding this comment.
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.
examples/misc/anchored_artists.py
Outdated
|
||
plt.draw() |
There was a problem hiding this comment.
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.
60619db
to
2bf5414
Compare
@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. |
2bf5414
to
ef7c672
Compare
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.
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
|
What exactly would be the purpose of |
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? |
@ImportanceOfBeingErnest I actually don't know how to build the docs locally. Will figure it out and let you know. |
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. |
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. |
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.) |
@ImportanceOfBeingErnest Tried it, ran into the issue of no such package |
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 |
@ImportanceOfBeingErnest. I was basically looking for |
No files were delete, instead, a clarifying comment was added to indicate why there are nearly duplicate files sitting around. [ci skip] [skip ci]
253bc73
to
6f7388c
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍
Thanks for the effort on this @madphysicist |
There seem to be a conflict, please backport manually |
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 reasonableanchored_bbox
examples up to04
. I did remove the clearly obsoletemisc/anchored_artists.html
and renamedaxes_grid1/simple_anchored_artists.html
toaxes_grid1/anchored_artists.html
. Hopefully the changes I made to the comments and docstrings in the example came out as an improvement.