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

Imviz astrowidgets API: save #759

Merged
merged 2 commits into from
Aug 17, 2021
Merged

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Aug 4, 2021

Description

This pull request is to implement the save method as per astrowidgets API definition.

Fixes #719

xref #632

Would be nice to have some inputs from @maartenbreddels though I think this PR is still acceptable without:

Screenshots

What you actually see on browser:

Screenshot 2021-08-04 182202

What it saves out:

myimage1

I did blink to another image, and it does save out the one that is displayed.

myimage2

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 change log needed? If yes, is it added to CHANGES.rst?
  • Is a milestone set? Milestone is only currently required for PRs related to Imviz MVP.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@pllim pllim added this to the Imviz 1.0 milestone Aug 4, 2021
@github-actions github-actions bot added documentation Explanation of code and concepts imviz labels Aug 4, 2021
@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #759 (a2d80b5) into main (cf3a026) will decrease coverage by 0.18%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #759      +/-   ##
==========================================
- Coverage   63.27%   63.08%   -0.19%     
==========================================
  Files          65       65              
  Lines        4324     4329       +5     
==========================================
- Hits         2736     2731       -5     
- Misses       1588     1598      +10     
Impacted Files Coverage Δ
jdaviz/configs/imviz/helper.py 97.05% <20.00%> (-1.66%) ⬇️
...configs/default/plugins/data_tools/file_chooser.py 65.71% <0.00%> (-3.43%) ⬇️

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 cf3a026...a2d80b5. Read the comment docs.

@ojustino ojustino assigned ojustino and unassigned ojustino Aug 5, 2021
@ojustino
Copy link
Contributor

ojustino commented Aug 5, 2021

If I'm able to review, I can give this a pass. I tried using both the save button in the toolbar and the programmatic method with imviz.save().

I have a few comments.

  • I did not get the "multiple downloads" alert on Firefox but did get it on Edge. I agree that this would be a nice thing to fix.
  • I agree that it would be good to not trigger a file dialog after the user runs imviz.save().
  • My images had the same gray border at their top and right edges as @pllim's. Are these intentional?

@pllim
Copy link
Contributor Author

pllim commented Aug 5, 2021

Re: Gray border -- Maybe @mariobuikhuizen can comment?

@pllim pllim requested a review from eteq August 5, 2021 21:25
@ojustino
Copy link
Contributor

Am I OK to add myself as a reviewer? Depending on how important it is to solve the gray border question, I can give approval since it seems like the other problems come from elsewhere.

@pllim pllim requested a review from ojustino August 16, 2021 20:02
@pllim
Copy link
Contributor Author

pllim commented Aug 16, 2021

@ojustino , feel free! And looks like we forgot to update the codeowners file when you joined. Will fix! See #789.

@rosteen
Copy link
Collaborator

rosteen commented Aug 16, 2021

@ojustino My two cents is that you should always feel free to add yourself as a reviewer.

Copy link
Contributor

@ojustino ojustino left a comment

Choose a reason for hiding this comment

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

The save process went smoothly for me. I'm curious about how to eliminate the gray borders and it would be nice to see the test in action, but I don't think either should prevent a merge.

@pllim
Copy link
Contributor Author

pllim commented Aug 16, 2021

how to eliminate the gray borders

I think that is a question for @maartenbreddels or @mariobuikhuizen . This API simply exposes this functionality from bqplot:

https://github.com/bqplot/bqplot/blob/b18af05244d3f4c592e09be46a4334584ab2600f/bqplot/figure.py#L164

Could be the VueJS stuff needs fixing, or maybe bqplot; I don't know.

@rosteen
Copy link
Collaborator

rosteen commented Aug 17, 2021

FYI it looks like the gray border doesn't occur on my setup - even if zoomed out to where there are areas of no data around the edges, the saved image has the same color as the viewer does in those areas. So my thought that the gray happens where there is no data seems to be incorrect. My environment (on Mac OSX, firefox):

astropy                           4.2.1
bqplot                            0.12.30                       /Users/rosteen/opt/anaconda3/envs/viz_dev/lib/python3.9/site-packages
bqplot-image-gl                   1.4.3
glue                              0.13
glue-astronomy                    0.2
glue-core                         1.0.1.dev81+g5978a927         /Users/rosteen/projects/glue
glue-jupyter                      0.7
glue-vispy-viewers                1.0.2
gwcs                              0.16.1
ipydatawidgets                    4.2.0
ipygoldenlayout                   0.4.0
ipyvue                            1.5.0
ipyvuetify                        1.7.0
ipywidgets                        7.6.3
jdaviz                            1.2.dev395+g2384547.d20210810 /Users/rosteen/projects/jdaviz
jupyter-client                    6.1.12
jupyter-contrib-core              0.3.3
jupyter-core                      4.7.1
jupyter-server                    1.8.0
matplotlib                        3.4.2
numpy                             1.20.3
photutils                         1.1.0
Pillow                            8.2.0
specutils                         1.3.dev121+gf0686ed0          /Users/rosteen/projects/specutils
traitlets                         5.0.5
voila                             0.2.10
widgetsnbextension                3.5.1

Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

Approving, since everything worked for me and it seems the fix to the gray border may be upstream.

@pllim pllim merged commit 25a6f80 into spacetelescope:main Aug 17, 2021
@pllim pllim deleted the somebody-save-me branch August 17, 2021 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Explanation of code and concepts imviz Ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to ask save_png not to pop up a file dialog when filename is given? Imviz astrowidgets API MVP: save
3 participants