Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
chahak13 commentedOct 11, 2022
@oscargus I've updated |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
oscargus commentedOct 11, 2022
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 commentedOct 11, 2022
Thanks for the suggestions! I'll make a few changes accordingly and update. Regarding extracting only dict keys, I think having |
chahak13 commentedOct 12, 2022
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! |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
chahak13 commentedOct 12, 2022
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! |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
chahak13 commentedOct 14, 2022
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 commentedOct 15, 2022
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Add other suggestions mentioned in the PR.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 commentedOct 20, 2022
@chahak13 Something did not go quite right with this rebase.... |
chahak13 commentedOct 20, 2022
Yes, I just realised that. Redoing this, sorry about that |
tacaswell commentedOct 20, 2022
No worries! @ me if you want help. |
`set_units` calls the `_update_axisinfo` function internally, so thecall here was redundant.
chahak13 commentedOct 20, 2022
@tacaswell I think that clears the rebase issue. Can you please check and confirm once? Thanks! |
tacaswell commentedOct 20, 2022
Rebase looks good and manually testing it works locally for me. |
chahak13 commentedOct 20, 2022
Awesome. Thank you all for your help! :) |
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.
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.
* 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.
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
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).