Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

DOC: Improve inverted axis example#28055

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
rcomer merged 1 commit intomatplotlib:mainfromtimhoffm:example-invert-axes
Apr 12, 2024

Conversation

@timhoffm
Copy link
Member

Closes#28050.

@github-actionsgithub-actionsbot added the Documentation: examplesfiles in galleries/examples labelApr 10, 2024
@story645
Copy link
Member

story645 commentedApr 10, 2024
edited
Loading

Since this looks like two ways to do the same thing, while I'm not a fan of multiple images in examples think this is a great use case for it since one thumbnail covers both methods. I'm thinking linkable subsections for:

  1. Recommended method

  2. Other method where we can write the caveats for when to use this method.

I'm hoping that subsection headings will prime folks to more easily find the one line of code that does the thing that way in each section.

Copy link
Member

@rcomerrcomer left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up! I had been going back and forth on whether and how to still include theset_xlim version, but this solves it very neatly.

I had been thinking along similar lines to@story645 to put two figures in with some explanation in between, but all I could come up with for when to useset_xlim was "if this is a context where you would have fixed the limits regardless of direction". That doesn’t really warrant a whole paragraph and the new axes titles cover this I think.

I’m approving as definitely better than it was but will also leave open for a bit in case others want to discuss further.

@rcomerrcomer added this to thev3.8-doc milestoneApr 11, 2024
@story645
Copy link
Member

story645 commentedApr 11, 2024
edited
Loading

That doesn’t really warrant a whole paragraph and the new axes titles cover this I think.

I think a sentence is fine for a new subsection, doesn't need to be a paragraph. My reasoning for splitting it into two sections is that:
1).the code kinda gets lost/harder to follow the more there is of it. Like here you've gotta match the axes to the method and then find the line so it's a two hops instead of 1
2) provides a way to more clearly identify a preferred/recommended method. I feel like putting em both in the same figure denotes that they're at the same level.

Which like clearly I've got stronger than I thought feelings about this & like I can put in a follow up advocating for this. Not gonna block but also not approving b/c while I agree it's better in that it documents something not documented, I'm worried that adding the second version in this manner makes it a little harder to figure out the "what should I do" from this example.

@rcomer
Copy link
Member

  1. provides a way to more clearly identify a preferred/recommended method. I feel like putting em both in the same figure denotes that they're at the same level.

When I opened the issue I was obviously thinkingset_inverted is the "right" way to do this but actually if you want fixed limits then usingset_xlim would be the right way. So I'm not sure I'd say one should be preferred in general, but that it depends on your use-case.

@timhoffmtimhoffm marked this pull request as draftApril 11, 2024 18:03
@timhoffm
Copy link
MemberAuthor

I still believe one figure is better, because you have the two code variants right next to each other, which my ales it easier to compare.

we can however make the “two variants” situation more visible in the description.

Will update later.

@story645
Copy link
Member

story645 commentedApr 11, 2024
edited
Loading

I still believe one figure is better, because you have the two code variants right next to each other, which my ales it easier to compare.

But here the image is identical, so I'm not sure that helps. And there's a balance in that while visually it may be easier to compare, I think the code can be harder to scan for which piece of code does the exact thing.

So I'm not sure I'd say one should be preferred in general, but that it depends on your use-case.

Sure, but like I think if we're not communicating when to use what, then it leaves the user super confused on why we have two ways to do the same thing.

@rcomer
Copy link
Member

But here the image is identical

They are not identical, as the fixed limit version has narrower limits. Perhaps that would be more obvious if they were one above each other instead of side-by-side?

@timhoffmtimhoffm marked this pull request as ready for reviewApril 11, 2024 19:23
@timhoffm
Copy link
MemberAuthor

Let's discuss based on this version.

@github-actionsgithub-actionsbot added the Documentation: APIfiles in lib/ and doc/api labelApr 11, 2024
@story645
Copy link
Member

story645 commentedApr 11, 2024
edited
Loading

But here the image is identical

They are not identical, as the fixed limit version has narrower limits. Perhaps that would be more obvious if they were one above each other instead of side-by-side?

Um these look pretty much identical to me ( realizing cause I had first thought that y was being inverted, even with the title 🙁, so yeah maybe stacked would help - but even then I think it's a bit of a you have to know what you're looking for problem, ie you have to know that auto scaling has margins to really see it in the image.)

Screenshot_20240411-163113.png

I'm less hung up on them being in one figure now cause yeah code that's short enough to fit on one phone screen is probably easily scannable, but I'm still not convinced that there's a benefit either...

Like I think if you're going w/ one figure, then you may as well invert x using one method and y using the other and label accordingly on each axis.

@timhoffm
Copy link
MemberAuthor

Sort of the point here is that both methods yield similar results: Both have inverted x-axis. The difference fixed vs. autoscaled axes is not immediately visible. We could use a larger range for the fixed-limits case, so that one clearly sees these are not autoscaled, but do we want to draw attention to this?

On one or two plots:
With two separate plots, the code blocks and figures are further apart and thus harder to compare. IMHO this is a disadvantage no matter whether we want to emphasize both variants result in similar plots, or whether we want to emphasize the difference in limits from auto-scaling vs. fixed limits.

@timhoffm
Copy link
MemberAuthor

timhoffm commentedApr 11, 2024
edited
Loading

Used x, y as variables and "decreasing x ⟶" as xlabel to draw more attention to the x axis.

To put even more emphasis, we could color xticks/xlabel e.g. orange, but I'm hesitant that the additional commands add too much clutter.

Edit: It would also help to be able to highlight important code lines. -> Create a feature request atsphinx-gallery/sphinx-gallery#1283.

@story645
Copy link
Member

story645 commentedApr 12, 2024
edited
Loading

the code blocks and figures are further apart and thus harder to compare.

Hmm a point I think I maybe am making badly is that I don't think the goal should be to set up a comparison of results - between the similarity of image and similarity of code, like the visible difference is super nuanced.

The reason I'm advocating instead for the separate sections is precisely to emphasize the differentiation/comparison on use case:

  1. invert axes with fixed limits
  2. invert axes with auto limits

@jklymak
Copy link
Member

The above is pretty clear.

If I were doing this,

a) I'd have a third subplot without the inverted axes
b) It sometimes happen that you want to invert the x axis, but itoften happens that you want to invert the y axis, at least in oceanography.

@timhoffm
Copy link
MemberAuthor

@story645@jklymak I yield here. My motivation and scope was a reasonable quick fix for#28050, which I believe is fulfilled. Feel free to bikeshed further without me.

jklymak reacted with thumbs up emoji

story645
story645 previously requested changesApr 12, 2024
Copy link
Member

@story645story645 left a comment

Choose a reason for hiding this comment

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

:/ I think "what will the user take away from this example" is more than bikeshedding, but I guess I'm ok w/ self merging and then me and Jody can hash things out on a follow up.

The request changes that you can dismiss is what's up with the changes to the API index in this PR?

timhoffm reacted with thumbs up emoji
@github-actionsgithub-actionsbot removed the Documentation: APIfiles in lib/ and doc/api labelApr 12, 2024
@rcomer
Copy link
Member

Let's get this in...

@rcomerrcomer merged commit9955a7c intomatplotlib:mainApr 12, 2024
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestApr 12, 2024
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestApr 12, 2024
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestApr 12, 2024
@timhoffmtimhoffm deleted the example-invert-axes branchApril 12, 2024 12:21
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@rcomerrcomerrcomer approved these changes

@story645story645story645 left review comments

Assignees

No one assigned

Labels

Documentation: examplesfiles in galleries/examples

Projects

None yet

Milestone

v3.8-doc

Development

Successfully merging this pull request may close these issues.

[Doc]: Invert Axes example sets x-lims directly

4 participants

@timhoffm@story645@rcomer@jklymak

[8]ページ先頭

©2009-2025 Movatter.jp