user API for export plot plugin #1657

kecnry · 2022-09-19T17:01:54Z

Description

This pull request extends on #1401 and provides user-API access to the export plot plugin.

Plugin User API docs

TODO:

add PR to changelog if before next release, otherwise add new entry (waiting until PR is accepted otherwise to avoid constant merge conflicts)

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge.

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?
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2022-09-19T17:12:48Z

Codecov Report

Base: 87.11% // Head: 87.07% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (d3c8b0b) compared to base (c8fc0a7).
Patch coverage: 42.85% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1657      +/-   ##
==========================================
- Coverage   87.11%   87.07%   -0.05%     
==========================================
  Files          95       95              
  Lines        9891     9902      +11     
==========================================
+ Hits         8617     8622       +5     
- Misses       1274     1280       +6

Impacted Files	Coverage Δ
...configs/default/plugins/export_plot/export_plot.py	`56.00% <42.85%> (-8.29%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

pllim · 2022-09-19T17:39:14Z

AFAIK this will pop up a file dialog regardless. Are you able to overwrite the filename the dialog prompts you via API? I don't remember. It's been a while since #929

kecnry · 2022-09-19T17:56:44Z

Right, it should overwrite the default name in the file dialog, but the file dialog is still raised. Hopefully we can eventually fix #929 and this will then just take the user input from the API without the need for the dialog.

kecnry · 2022-09-19T18:27:52Z

Should test coverage be added here or is that blocked because of the dialog popup? (Note the uncovered lines were uncovered before, now they've just been moved around a little to be available without the vue_ method).

pllim · 2022-09-19T19:38:01Z

It is impossible to test due to the pop up. It pops up and will hold the CI until it times out.

jdaviz/jdaviz/configs/imviz/tests/test_astrowidgets_api.py

Line 18 in 1639526

@pytest.mark.skip(reason="Cannot test due to file dialog popup")

pllim · 2022-09-19T20:32:47Z

jdaviz/configs/default/plugins/export_plot/export_plot.py

        """
+        if filetype is None:
+            if filename is not None:
+                filetype = filename.split('.')[-1]


Sometimes people just give the name without suffix because they are used to other software attaching suffix for them.

Maybe the code to auto populate the filename should be its own private function, so at least we can test that part. The only thing we cannot test is viewer.figure.save_png() call.

that's fine, this logic should handle that scenario as expected, right? I guess the only problem would be if they did filename='blah.randomextension' which would raise a NotImplementedError instead of saving to blah.randomextension.png.

Should I instead only apply the filetype if its either png or svg and otherwise default to png? But then filename='blah.jpeg' would silently write to blah.jpeg.png.

Silent fallback means they still get the image, better than error from trying to save to an unsupported extension? Unless you want upstream to deal with that instead?

I always prefer (helpful) errors than unexpected magic/fallbacks myself 🤷

When I don't give file extension to Photoshop, it doesn't crash. It saves my stuff in some default format. I think users might be looking for similar established behaviors. 🤷

Works for me

done (default to png when no extension is provided in the filename by checking for '.' in filename)

By the way, would mimetypes be useful for your purpose? https://docs.python.org/3/library/mimetypes.html

In [10]: mimetypes.guess_type('dummy_gwcs.png') Out[10]: ('image/png', None) In [11]: mimetypes.guess_type("test_spec") Out[11]: (None, None)

interesting idea. I think it might be overkill here since we're passing to one of two backend calls... but maybe if we generalize that in the future to avoid the file dialog, this would be a good direction to take.

pllim

Just minor comments, otherwise LGTM. Thanks!

jdaviz/configs/default/plugins/export_plot/export_plot.py

pllim · 2022-09-30T14:18:36Z

Still need a change log. FYI.

kecnry · 2022-09-30T14:29:07Z

Yes, I'll add the change log between approval and merge (since there are still a few outstanding API PRs that touch the same change log entry and cause constant conflicts otherwise)

rosteen

LGTM

Co-authored-by: P. L. Lim <[email protected]>

kecnry marked this pull request as ready for review September 19, 2022 19:57

kecnry requested review from duytnguyendtn, rosteen, javerbukh, ojustino and pllim as code owners September 19, 2022 19:57

kecnry added this to the 2.11 milestone Sep 19, 2022

kecnry added the plugin Label for plugins common to multiple configurations label Sep 19, 2022

pllim reviewed Sep 19, 2022

View reviewed changes

pllim reviewed Sep 30, 2022

View reviewed changes

jdaviz/configs/default/plugins/export_plot/export_plot.py Outdated Show resolved Hide resolved

jdaviz/configs/default/plugins/export_plot/export_plot.py Outdated Show resolved Hide resolved

jdaviz/configs/default/plugins/export_plot/export_plot.py Outdated Show resolved Hide resolved

pllim approved these changes Sep 30, 2022

View reviewed changes

pllim added the Ready for final review label Sep 30, 2022

rosteen approved these changes Sep 30, 2022

View reviewed changes

kecnry and others added 4 commits September 30, 2022 11:32

user API for export plot plugin

e359978

Apply suggestions from code review

3e98e5f

Co-authored-by: P. L. Lim <[email protected]>

default to png when no extension provided in filename

023b18d

update change log entry

d3c8b0b

kecnry force-pushed the api-export-plot branch from a9bcbb4 to d3c8b0b Compare September 30, 2022 15:35

kecnry merged commit 033ea81 into spacetelescope:main Sep 30, 2022

kecnry deleted the api-export-plot branch September 30, 2022 15:49

kecnry mentioned this pull request Sep 30, 2022

[FEAT] Imviz: Documentation -- include list of API-settable options with examples (akin to ds9 XPA documentation) #1417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

user API for export plot plugin #1657

user API for export plot plugin #1657

kecnry commented Sep 19, 2022 •

edited

Loading

codecov bot commented Sep 19, 2022 •

edited

Loading

pllim commented Sep 19, 2022

kecnry commented Sep 19, 2022

kecnry commented Sep 19, 2022

pllim commented Sep 19, 2022

pllim Sep 19, 2022

pllim Sep 19, 2022

kecnry Sep 19, 2022

pllim Sep 19, 2022

kecnry Sep 19, 2022

pllim Sep 30, 2022 •

edited

Loading

rosteen Sep 30, 2022

kecnry Sep 30, 2022

pllim Sep 30, 2022

kecnry Sep 30, 2022

pllim left a comment

pllim commented Sep 30, 2022

kecnry commented Sep 30, 2022

rosteen left a comment

user API for export plot plugin #1657

user API for export plot plugin #1657

Conversation

kecnry commented Sep 19, 2022 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Sep 19, 2022 • edited Loading

Codecov Report

pllim commented Sep 19, 2022

kecnry commented Sep 19, 2022

kecnry commented Sep 19, 2022

pllim commented Sep 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

pllim commented Sep 30, 2022

kecnry commented Sep 30, 2022

rosteen left a comment

Choose a reason for hiding this comment

kecnry commented Sep 19, 2022 •

edited

Loading

codecov bot commented Sep 19, 2022 •

edited

Loading

pllim Sep 30, 2022 •

edited

Loading