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

Export plugin plots #2774

Merged
merged 25 commits into from
Apr 3, 2024
Merged

Conversation

cshanahan1
Copy link
Contributor

@cshanahan1 cshanahan1 commented Apr 1, 2024

Add ability to export plots from various plugins through export plugin.

@kecnry did much of this, and the changes I made on top of his are:

  1. Filtering out empty plots (since some plots, like aperture photometry, exist but are empty) so they're not displayed in the plugin.
  2. Test coverage

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@github-actions github-actions bot added imviz plugin Label for plugins common to multiple configurations labels Apr 1, 2024
Copy link

github-actions bot commented Apr 1, 2024

I don't want to be human! I want to see gamma rays!

(A special day message.)

@cshanahan1 cshanahan1 added this to the 3.9 milestone Apr 1, 2024
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 98.01980% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 88.93%. Comparing base (f1ba4a6) to head (bdfadfb).
Report is 18 commits behind head on main.

❗ Current head bdfadfb differs from pull request most recent head ba7f7df. Consider uploading reports for the commit ba7f7df to get more accurate results

Files Patch % Lines
jdaviz/app.py 87.50% 1 Missing ⚠️
jdaviz/core/template_mixin.py 97.67% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2774      +/-   ##
==========================================
+ Coverage   88.72%   88.93%   +0.20%     
==========================================
  Files         110      110              
  Lines       16353    16473     +120     
==========================================
+ Hits        14509    14650     +141     
+ Misses       1844     1823      -21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jdaviz/app.py Outdated Show resolved Hide resolved
jdaviz/core/template_mixin.py Show resolved Hide resolved
jdaviz/core/template_mixin.py Show resolved Hide resolved
jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved
jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved
jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved
@cshanahan1 cshanahan1 marked this pull request as ready for review April 2, 2024 17:16
@cshanahan1 cshanahan1 reopened this Apr 2, 2024
@cshanahan1
Copy link
Contributor Author

(whoops sorry i pressed close with comment instead of comment)

Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

@haticekaratay
Copy link
Contributor

This might be only on my end. I see a success message for export Exported to cubeviz_export.svg, but it doesn't show up on the current directory or downloads.

@cshanahan1
Copy link
Contributor Author

Try restarting the kernel? This was happening for me occasionally

@kecnry
Copy link
Member

kecnry commented Apr 2, 2024

This might be susceptible to the same problems as the viewer exporting as its using the same internals. We'll just have to live what that for now, I think

@haticekaratay
Copy link
Contributor

I can't confirm at this point if it works as it should, as clearing the cache, restarting the kernel, and reinstalling the branch didn't solve the problem on my end.

@cshanahan1
Copy link
Contributor Author

@haticekaratay does exporting other items work, is it just the plots that are not working?

@haticekaratay
Copy link
Contributor

haticekaratay commented Apr 2, 2024

@haticekaratay does exporting other items work, is it just the plots that are not working?

Yes, I can export other items. I guess we can merge it as both you and @kecnry confirms that it works. Or another dev might look into it.

@rosteen
Copy link
Collaborator

rosteen commented Apr 2, 2024

I also don't see where the PNG file ends up - I'm trying to rebase on top of #2782 to see if that fixes it, but the rebase is more painful than I expected and I'm out of time tonight. I'll try to continue in the morning.

@cshanahan1
Copy link
Contributor Author

cshanahan1 commented Apr 3, 2024

i think i have a hunch about what is going on. What i first noticed was that the stretch histogram is not actually populated with meaningful values until plot_options is opened. If you open the data analysis plugins from the icon not in the viewer, plot_options doesn't open automatically, so the histogram is 'empty'. Once you open that plugin and the histogram is fully created, you should be able to export. This is why I was seeing it working and others weren't - I was able to export stretch_histogram because I opened the plugin tools from the viewer, which automatically opens plot_options.

This isn't the exact reason though - it's not because the histogram is 'empty' that it won't save, there is something about opening it and rendering it for display that allows it to be saved, and if plot options isn't opened first, this doesn't happen. If the histogram were truly 'empty', it wouldn't appear in the export plugin as available for export at all (the filter that is done for plugin plot select checks figure.marks, and in this case it isn't actually empty even though the values don't reflect the data until the plugin is opened).

The fact that plot_options isn't opened automatically to render the figure for display before trying to export it is what is causing it to fail when exporting, I think. You can try this programmatically too - the plugin plot _obj is able to be inspected but not saved until you actually display in in the notebook.

@kecnry
Copy link
Member

kecnry commented Apr 3, 2024

This is caused by the same thing that causes the viewers to fail to export until the app is rendered 🐱. I tried wrapping the call with plugin.as_active(), but I think it still needs to be displayed somewhere in order to fully render. I have a few ideas how to work around this - I'll investigate and report back.

@rosteen
Copy link
Collaborator

rosteen commented Apr 3, 2024

Unfortunately (for that theory) I wasn't trying to save out the plot options histogram, I had run aperture photometry to make a curve of growth plot and was trying to save that, so it was definitely populated.

@rosteen
Copy link
Collaborator

rosteen commented Apr 3, 2024

Also, this might need a rebase to main - I see a bunch of commits from Hatice that I assume aren't supposed to be here. Unless this was building off something else that wasn't merged in addition to Kyle's WIP.

@kecnry
Copy link
Member

kecnry commented Apr 3, 2024

See cshanahan1#1 for a proposed "solution"/workaround.

@cshanahan1
Copy link
Contributor Author

Also, this might need a rebase to main - I see a bunch of commits from Hatice that I assume aren't supposed to be here. Unless this was building off something else that wasn't merged in addition to Kyle's WIP.

I was working off Kyle's WIP branch which was working off Hatice's WIP branch. I merged main into my branch yesterday too, so maybe that's it. Git history confuses me...

@cshanahan1
Copy link
Contributor Author

Unfortunately (for that theory) I wasn't trying to save out the plot options histogram, I had run aperture photometry to make a curve of growth plot and was trying to save that, so it was definitely populated.

Ok so I was able to reproduce this, but then refreshing the kernel made the issue go away. Then i refreshed a few more times and it happened again. I can't figure out why it fails only sometimes for me. Does it always fail to export for you?

@cshanahan1
Copy link
Contributor Author

It does seem to happen though consistently when I close the aperture photometry plugin, so hopefully Kyle's PR should fix that

@rosteen
Copy link
Collaborator

rosteen commented Apr 3, 2024

Ahh yes that's it, I confirmed that if I close the aperture photometry plugin the export of the plot fails. If it's open, the save dialog appears as expected.

render selected plot for export offscreen
@haticekaratay
Copy link
Contributor

I tested with Cubeviz, and it exports the plots as expected.
image

@haticekaratay
Copy link
Contributor

Sorry, I just realized the failing test. Approved to be merged after a fix to that.

@cshanahan1 cshanahan1 merged commit 77d5099 into spacetelescope:main Apr 3, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imviz plugin Label for plugins common to multiple configurations Ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants