Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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
@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.
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. |
Thanks for the suggestions! I'll make a few changes accordingly and update. Regarding extracting only dict keys, I think having |
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.
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.
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? |
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>
@chahak13 Something did not go quite right with this rebase.... |
Yes, I just realised that. Redoing this, sorry about that |
No worries! @ me if you want help. |
`set_units` calls the `_update_axisinfo` function internally, so thecall here was redundant.
@tacaswell I think that clears the rebase issue. Can you please check and confirm once? Thanks! |
Rebase looks good and manually testing it works locally for me. |
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
pytest
passes).flake8-docstrings
and 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).