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

fixing 3d ticks direction#22517

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

Closed
ghost wants to merge23 commits intomatplotlib:mainfromunknown repository
Closed

fixing 3d ticks direction#22517

ghost wants to merge23 commits intomatplotlib:mainfromunknown repository

Conversation

ghost
Copy link

fixing 3d tick:
#21729

seems to be working:
ax.set_xlabel(r"$x$")
ax.set_ylabel(r"$y$")
ax.set_zlabel(r"$z$")
ax.tick_params(axis='x', which='major', direction='inout')
ax.tick_params(axis='y', which='major', direction='in')
ax.tick_params(axis='z', which='major', direction='out')
Figure_1

@ghost
Copy link
Author

it seems some image comparisons tests fail. this is most likely because before it was 'inout' displayed even though default is 'out'? That would be my guess.

@ghost
Copy link
Author

I looked at the failing tests - and it seems all of them are the "out" default, which was before "inout" as it was done incorrectly. So the fix seems to be correct.

wireframe3dzerorstride
wireframe3dzerorstride-expected

@oscargusoscargus added status: needs workflow approvalFor PRs from new contributors, from which GitHub blocks workflows by default. topic: ticks axis labels labelsFeb 24, 2022
@timhoffm
Copy link
Member

Thanks, making this configurable is good. I think the previous state was just hard-coded to an 'in-out' solution and ignoring the tick params settings.

There's a bit of trouble with the defaults: On 2D we have classesXTick andYTick, which take their defaults fromrcParams['xtick.direction'] andrcParams['ytick.direction'].
On 3D all directions populate their axis withXTick and take their value fromrcParams['xtick.direction']. Note that we neither have aZTick class norrcParams['ztick.*'].
Also note that this inconsistency has existed for other tick defaultsrcParams['xticks.*'] (e.g.xticks.color) as well. So this is not new, it just becomes additionally visible in the tick direction now that it is not hard-coded anymore but followsrcParams['xticks.direction'].

There are several ways one could go forward, however I think the following is the best for now:
Patch the 3D ticks to default to a hard-coded internal 'inout', but let it be overwritten bytick_params(direction=...).

Advantages:

  • We do not modify existing plots/defaults
  • Users can now change the direction viatick_params(direction=...).
  • We don't have to get into the business of untangling the rcParams mess for 3d ticks right now.
  • This is still open for future changes. Any additional change we could make here now can be added later with the same amount of user impact.

Disadvantages:

  • The tick direction is still not configurable via rcParams.

I have not looked into the details how the 3D tick default direction could be patched, but assume that it can be done by a few lines of code.
This is a bit hacky, but every other solution will require either significant rewrites for 3D tick handling, or changed default behavior or have inconsistencies in rcParams usage; likely two or all three of them.

anntzer reacted with thumbs up emoji

@ghost
Copy link
Author

@timhoffm, so you want me to add code, which will be removed anyway just to postpone fixing the tests? I can't say I see any advantages in that, just disadvantages.

@timhoffm
Copy link
Member

The suggestions is not motivated by the tests. This is about user-facing behavior and API.

Before the tick direction was hard-coded. If we now do nothing special, the default tick direction would be determined byrcParams['xticks.direction'] for all axes.

  1. This would change the default behavior from 'out' to 'inout'. We are very hesitant with such changes because it changes the appearance of user figures. Even if that's acceptable, we have an additional complication:
  2. The ticks directions for all 3D axes are taken from 'xticks.direction' (because for some reason they are all XTick instances). In contrast, for 2D we use 'xticks.direction' and 'yticks.direction'.
    There are again two possible ways out:
    3a) Create a new3dticks.direction rcParam (naming t.b.d.) - Possibly, what's a good default for 2D is not necessarily a good default for 3D.
    3b) Change the 3D ticks to XTicks, YTicks and create a new ZTicks class and add 'zticks.*' rcParams.
    Both 3a) and 3b) would require quite a bit internal refactoring.

In summary, as soon as we start using rcParams for 3D, we are getting undesired API/behavior in the intermediate solutions. Only if we fully implement 3a or 3b, we get back to a reasonable API. But that's requires quite a bit internal restructuring, and we would need to decide whether 3a or 3b is the desired solution. IHMO this is beyond the scope of this PR and could significantly delay the adoption of this PR.

@ghost
Copy link
Author

Thank you for explanations Tim. Yes, indeed it seems there is much more work on this. I just wanted to add ability to do the ticks 'in' as sometimes they went out and got too close to text and it made it look a bit weird - not acceptable for publication. By adding ability setting ticks 'in', it would resolve a lot of headaches in the future.

@oscargus
Copy link
Member

oscargus commentedMar 2, 2022
edited
Loading

I may be late on the ball here and not following all the discussions properly, but wouldn't it make sense tonot change the test images, but to use the same styles for them? And then add one image, like the one in the opening post of this PR to prove the functionality?

This is from the aspect of saving repository space by not updating test images unless absolutely necessary.

(Being able to configure it is 👍🏻 )

@ghost
Copy link
Author

Well, quite frankly I am bit lost here.

I may be late on the ball here and not following all the discussions properly, but wouldn't it make sense tonot change the test images, but to use the same styles for them? And then add one image, like the one in the opening post of this PR to prove the functionality?

This is from the aspect of saving repository space by not updating test images unless absolutely necessary.

(Being able to configure it is 👍🏻 )

@ghostghost closed thisMar 2, 2022
@ghost ghost deleted the tick3dfix branchMarch 2, 2022 15:58
@timhoffm
Copy link
Member

timhoffm commentedMar 3, 2022
edited
Loading

@oscargus: Yes.

@Pepe78: Have we scared you away? If so, sorry for that. The large user base and legacy code makes it sometimes hard to introduce changes or even only fixes. But your PR is a good addition and there's not much lacking for it to be merged.

@ghost
Copy link
Author

ghost commentedMar 3, 2022
edited by ghost
Loading

Hah. No. I just realized that it takes shorter time to write my own libraries than checking in three and half lines of code fix into matplotlib:
https://github.com/pepe78/subdivision_opencl
(to be specific:https://github.com/pepe78/subdivision_opencl/blob/main/display.py)

as i am currently considering switching languages anyway:
https://github.com/pepe78/python-performance

I guess i will be most likely using other library in future for displaying graphs anyway (maybe rust?).

@ghost
Copy link
Author

Sorry that I wasn't more helpful. You guys can take the code and check it in if you know how to do it properly. I obviously didn't.

@WeatherGod
Copy link
Member

WeatherGod commentedMar 3, 2022 via email

Well, you will still get credit for your contribution, we'll just have tofigure out the rest. We are thankful for submitting even this much.As for performance, just letting you know that your choice of language fordata processing can be independent of your choice of language for plotting.I often generate data in very fast languages like C++ or Fortran, save themout as netcdfs, and then can quickly plot the data using matplotlib andxarray in python. And if you want to use that link as a guide for languageperformance (dictionary performance is just one metric of many toconsider), then you'd be happy to know that matplotlib and numpy can workwith PyPy.Ultimately, matplotlib's position is that we just want users to make goodplots from their data, regardless of the library you use. So happy plotting!
On Wed, Mar 2, 2022 at 7:37 PM Peter Taraba ***@***.***> wrote: Sorry that I wasn't more helpful. You guys can take the code and check it in if you know how to do it properly. I obviously didn't. — Reply to this email directly, view it on GitHub <#22517 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACHF6B4T7DSPQFRXQBSJRDU6ACTDANCNFSM5O5D4NMQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

@ghost
Copy link
Author

Thanks :) - as I said - I wished I was more helpful. I understand that matplotlib is going through a lot of changes (I actually looked at a lot of code) and I saw you guys are trying to figure the best ways (which is often hard on a moving train). I just felt I am going to slow you down :). And yes, matplotlib can be used with other languages as well. Happy plotting (I will still be using matplotlib a lot in future).

timhoffm and oscargus reacted with thumbs up emoji

@timhoffmtimhoffm self-assigned thisMar 3, 2022
@timhoffm
Copy link
Member

Will try to pick this up after#22587 is in.

@ghost
Copy link
Author

Just FYI - matplotlib's 3D graphs got into my latest publications:
https://www.sciencedirect.com/science/article/pii/S0167691122000329

So Thank you for all the hard work to make them look pretty!

oscargus reacted with thumbs up emoji

This pull request wasclosed.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees

@timhoffmtimhoffm

Labels
status: needs workflow approvalFor PRs from new contributors, from which GitHub blocks workflows by default.topic: ticks axis labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@timhoffm@oscargus@WeatherGod@QuLogic@peta78

[8]ページ先頭

©2009-2025 Movatter.jp