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

Set figure options dynamically#24141

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
timhoffm merged 9 commits intomatplotlib:mainfromchahak13:z_axis_3d_support
Oct 21, 2022

Conversation

chahak13
Copy link
Contributor

As mentioned in#23566 , for the figure options, things were hard coded for the X and Y axes but were not implemented for the Z axis. With this commit, all the options in the Figure options dialogue box are generated dynamically based on the axes present in the figure. This removes all the hard coded part and should make it more modular to make further changes.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

Ankush-Chander reacted with rocket emoji
@chahak13
Copy link
ContributorAuthor

@oscargus I've updatedfigureoptions.py to generate all the options dynamically based on the axes present. I tried running all the options through the dialog box and everything seems to be working as it was before but includes the Z axis now, whenever present. Can you take a look once and let me know if you have any suggestions? Thanks!

@oscargus
Copy link
Member

Nice work!

Some minor things. I think that either one just get the keys once and for all, but maybe it also makes sense to have the axis_map variable as well as one can iterate over both key and value which makes sense sometimes.

I'll try to test it as well, but not tonight.

@chahak13
Copy link
ContributorAuthor

Thanks for the suggestions! I'll make a few changes accordingly and update. Regarding extracting only dict keys, I think havingaxis_map around would be more useful. Also, given that the keys are thegetattr objects, I can change all those calls to values of that dict instead which would make it cleaner IMO. Let me know what you think!

@chahak13
Copy link
ContributorAuthor

I made a few changes as you suggested. I've kept one conversation open if you want to discuss about it. Let me know if you have any other thoughts. Thanks!

@chahak13
Copy link
ContributorAuthor

Thanks,@timhoffm ! I changed the naming and used the axis object methods to make it more consistent. Let me know if it looks good. Thanks!

@tacaswelltacaswell added this to thev3.7.0 milestoneOct 12, 2022
@chahak13
Copy link
ContributorAuthor

The ubuntu tests are failing for some reason. I'm on an arch machine and everything seems to be working fine. Any reason why this is happening?

@timhoffm
Copy link
Member

Can you please rebase on main? We had some CI breakages lately, but I think all of them are fixed on main now.

Previously, for the figure options, things were hard coded for the X andY axes but were not implemented for the Z axis. With this commit, allthe options in the Figure options dialogue box are generated dynamicallybased on the axes present in the figure. This removes all the hard codedpart and should make it more modular to make further changes.
The value in `axis_map` is the axes object for that axis so we can usethat directly to get the object instead of using `getattr`s by loopingover the items of the dictionary. This also removes the need to have an`axis_converter` dictionary.
Add other suggestions mentioned in the PR.
chahak13and others added2 commitsOctober 20, 2022 11:41
Change axis name conversion to `.title` instead of `.upper` to handle more general axis names.Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Use private method of `axis` instead of using `getattr` on `axes` to set limits.Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@tacaswell
Copy link
Member

@chahak13 Something did not go quite right with this rebase....

@chahak13
Copy link
ContributorAuthor

Yes, I just realised that. Redoing this, sorry about that

@tacaswell
Copy link
Member

No worries! @ me if you want help.

`set_units` calls the `_update_axisinfo` function internally, so thecall here was redundant.
@chahak13
Copy link
ContributorAuthor

@tacaswell I think that clears the rebase issue. Can you please check and confirm once? Thanks!

@tacaswell
Copy link
Member

Rebase looks good and manually testing it works locally for me.

@chahak13
Copy link
ContributorAuthor

Awesome. Thank you all for your help! :)

@timhoffmtimhoffm merged commit5fe74c2 intomatplotlib:mainOct 21, 2022
ksunden added a commit to ksunden/matplotlib that referenced this pull requestOct 31, 2024
The replacement is the get/set_converter method.This includes changes to use the getter and the private setter in the qtfigure options dialog menu.The choice to use the private setter was a defensive one as the publicsetter prevents being called multiple times (though does short circuitif an identical input is provided, which I think is actually true here,therefore the public one is probably functional (and a no-op).)It is not clear to me on analysis how the unit information is or waslost. A quick test commenting out the two lines which resetconverter/units displayed no obvious detrimental effect to removingthose, suggesting that even if once they were necessary, they may nolonger be.These lines were last touched inmatplotlib#24141, though that really only generalizedthe code into a loop rather than copy/pasted x and y behavior.The original inclusion of resetting was inmatplotlib#4909, which indicated thatthe dialog reset unit info. AFAICT, that is no longer true, though Ihave not rigorously proved that.
ksunden added a commit to ksunden/matplotlib that referenced this pull requestOct 31, 2024
The replacement is the get/set_converter method.This includes changes to use the getter and the private setter in the qtfigure options dialog menu.The choice to use the private setter was a defensive one as the publicsetter prevents being called multiple times (though does short circuitif an identical input is provided, which I think is actually true here,therefore the public one is probably functional (and a no-op).)It is not clear to me on analysis how the unit information is or waslost. A quick test commenting out the two lines which resetconverter/units displayed no obvious detrimental effect to removingthose, suggesting that even if once they were necessary, they may nolonger be.These lines were last touched inmatplotlib#24141, though that really only generalizedthe code into a loop rather than copy/pasted x and y behavior.The original inclusion of resetting was inmatplotlib#4909, which indicated thatthe dialog reset unit info. AFAICT, that is no longer true, though Ihave not rigorously proved that.
tacaswell pushed a commit that referenced this pull requestOct 31, 2024
* Add explicit converter setting to AxisCloses#19229The replacement is the get/set_converter method.This includes changes to use the getter and the private setter in the qtfigure options dialog menu.The choice to use the private setter was a defensive one as the publicsetter prevents being called multiple times (though does short circuitif an identical input is provided, which I think is actually true here,therefore the public one is probably functional (and a no-op).)It is not clear to me on analysis how the unit information is or waslost. A quick test commenting out the two lines which resetconverter/units displayed no obvious detrimental effect to removingthose, suggesting that even if once they were necessary, they may nolonger be.These lines were last touched in#24141, though that really only generalizedthe code into a loop rather than copy/pasted x and y behavior.The original inclusion of resetting was in#4909, which indicated thatthe dialog reset unit info. AFAICT, that is no longer true, though Ihave not rigorously proved that.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic left review comments

@oscargusoscargusoscargus left review comments

@tacaswelltacaswelltacaswell approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.7.0
Development

Successfully merging this pull request may close these issues.

5 participants
@chahak13@oscargus@timhoffm@tacaswell@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp