Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide Matplotlib Axes checkbox #2128

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Conversation

jsub1
Copy link
Contributor

@jsub1 jsub1 commented Apr 24, 2020

This creates a checkbox that allows the user to hide 2D matplotlib axis. Allows to hide/show axis to included when exporting as script. Also adds test for hiding the axis and exporting.

@jsub1
Copy link
Contributor Author

jsub1 commented Apr 24, 2020

For issue #1883

@codecov
Copy link

codecov bot commented Apr 24, 2020

Codecov Report

Merging #2128 into master will increase coverage by 0.10%.
The diff coverage is 98.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2128      +/-   ##
==========================================
+ Coverage   87.86%   87.96%   +0.10%     
==========================================
  Files         246      246              
  Lines       22708    22726      +18     
==========================================
+ Hits        19952    19991      +39     
+ Misses       2756     2735      -21     
Impacted Files Coverage Δ
glue/viewers/image/viewer.py 93.58% <85.71%> (+0.87%) ⬆️
glue/viewers/matplotlib/viewer.py 95.83% <97.50%> (+1.91%) ⬆️
glue/viewers/histogram/qt/options_widget.py 87.09% <100.00%> (ø)
glue/viewers/image/qt/options_widget.py 84.00% <100.00%> (ø)
glue/viewers/matplotlib/mpl_axes.py 90.47% <100.00%> (+0.23%) ⬆️
glue/viewers/matplotlib/state.py 99.39% <100.00%> (+9.20%) ⬆️
glue/viewers/profile/qt/options_widget.py 100.00% <100.00%> (ø)
glue/viewers/scatter/qt/options_widget.py 89.74% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fcd828...62785cd. Read the comment docs.

@astrofrog astrofrog self-requested a review April 29, 2020 15:11
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I finally had a chance to take this for a spin, and it works well! However, two requests before we can go ahead and merge:

  • When axes are not shown, we should also remove the white padding around the image. If you take a look at the init_mpl function you'll see it calls freeze_margins. I think when you turn off the axes you might want to change that to freeze the margins to zero, and then change the frozen margins back to what they are currently if axes are turned back on.

  • Currently if you load the L1448 cube, and show velocity on the x axis and say RA on the y axis, the MatplotlibImageMixin._set_wcs method is still getting called even when the axes are hidden. In principle we could avoid this call, or at least exit early inside that method if the axes are now shown - and then if the axes are turned on again, we could explicitly call _set_wcs. This might make the slider even smoother for cases where the coordinate labels take a while to compute.

  • Could you double check that in astropy.visualization.wcs, the ticks and so on are not being computed/drawn when the axes are hidden as is done here? I just want to make sure it's not that they are being computed but then not shown.

  • It looks like there are some real code style issues and potentially real test failures?

Thanks!

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rebase and remove the last commit since that CI issue should now be fixed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants