Set figure options dynamically #24141
Conversation
|
@oscargus I've updated |
|
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! |
|
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! |
|
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. |
0632fbd to
357753c
Compare
Previously, 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.
The value in `axis_map` is the axes object for that axis so we can use that directly to get the object instead of using `getattr`s by looping over the items of the dictionary. This also removes the need to have an `axis_converter` dictionary.
357753c to
6e21419
Compare
Add other suggestions mentioned in the PR.
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. |
d12c1f2 to
372a837
Compare
`set_units` calls the `_update_axisinfo` function internally, so the call 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 qt figure options dialog menu. The choice to use the private setter was a defensive one as the public setter prevents being called multiple times (though does short circuit if 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 was lost. A quick test commenting out the two lines which reset converter/units displayed no obvious detrimental effect to removing those, suggesting that even if once they were necessary, they may no longer be. These lines were last touched in matplotlib#24141, though that really only generalized the code into a loop rather than copy/pasted x and y behavior. The original inclusion of resetting was in matplotlib#4909, which indicated that the dialog reset unit info. AFAICT, that is no longer true, though I have 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 qt figure options dialog menu. The choice to use the private setter was a defensive one as the public setter prevents being called multiple times (though does short circuit if 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 was lost. A quick test commenting out the two lines which reset converter/units displayed no obvious detrimental effect to removing those, suggesting that even if once they were necessary, they may no longer be. These lines were last touched in matplotlib#24141, though that really only generalized the code into a loop rather than copy/pasted x and y behavior. The original inclusion of resetting was in matplotlib#4909, which indicated that the dialog reset unit info. AFAICT, that is no longer true, though I have not rigorously proved that.
* Add explicit converter setting to Axis Closes #19229 The replacement is the get/set_converter method. This includes changes to use the getter and the private setter in the qt figure options dialog menu. The choice to use the private setter was a defensive one as the public setter prevents being called multiple times (though does short circuit if 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 was lost. A quick test commenting out the two lines which reset converter/units displayed no obvious detrimental effect to removing those, suggesting that even if once they were necessary, they may no longer be. These lines were last touched in #24141, though that really only generalized the code into a loop rather than copy/pasted x and y behavior. The original inclusion of resetting was in #4909, which indicated that the dialog reset unit info. AFAICT, that is no longer true, though I have 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).