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 (deprecates/replaces "Export Plot") #2722

Merged
merged 18 commits into from
Mar 8, 2024

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Feb 22, 2024

Description

This pull request implements the basic UI functionality of a future "Export" plugin, to replace the "Export Plot" plugin. This PR should not yet be merged until functionality from the "Export Plot" plugin is mapped over and properly deprecated, but can be merged before additional support is added as they are hidden under feature flags.

Without any feature flags enabled (viewer export only - note that cubeviz movie options should also be added underneath the format when functionality from export plot is mapped over):

Screen.Recording.2024-02-22.at.11.18.42.AM.mov

To enable feature flags:

exp = viz.plugins['Export']
exp._obj.dev_multi_support = True
exp._obj.dev_dataset_support = True
exp._obj.dev_subset_support = True
exp._obj.dev_table_support = True
exp._obj.dev_plot_support = True
exp = viz.plugins['Export'] # to pickup the new user API
exp.open_in_tray()
Screen.Recording.2024-02-22.at.11.19.14.AM.mov

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. If no, maintainer
    should add a no-changelog-entry-needed label.

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)?

@kecnry kecnry added the plugin Label for plugins common to multiple configurations label Feb 22, 2024
@kecnry kecnry added this to the 3.9 milestone Feb 22, 2024
@kecnry kecnry force-pushed the export-plugin-design branch from e681d48 to 5770da1 Compare February 22, 2024 16:17
@kecnry kecnry requested a review from Jenneh February 22, 2024 16:23
@Jenneh
Copy link
Collaborator

Jenneh commented Feb 23, 2024

I am concerned about this paradigm. Most export options are accessible from the thing (plot, table, image, etc) that is being exported. I worry that users will not innately know to look in this menu for export options. It would feel more natural to have an export icon right on each panel.

Perhaps there could be a button on each plot that opens and expands this panel?

Another note, There were some requests for the ability to export plots from within other plugins. It does not seem like this supports that functionality but that would be desirable.

@gibsongreen
Copy link
Contributor

One idea I can think of is, let's say the user extracts a spectrum. In the Export plugin, in the Data Selection, this would auto populate as it should. The extracted spectrum could be the default selection made by the plugin, since this is new data comparatively to the ERR or SCI data which they had from the original file loaded.

Another idea is, let's say the user extracts a spectrum and creates a moment map (or any combination of multiple pieces of new data). Instead of toggling the multi-select button on, if the default idea previously is desirable, the default in this case would use multi-select, and default to the selection of both the spectrum and the moment map for download.

@gibsongreen
Copy link
Contributor

gibsongreen commented Feb 23, 2024

The zip idea was mentioned in parking lot. I can imagine that if a user has several pieces of data the desire to export, that renaming each file would be painful. I wasn't sure if I misinterpreted, but I thought the default option is multiple items were selected, the renaming of the zip file would be what the filename input field would allow.

Whenever I've downloaded compressed folders, I am normally not thinking about the folder name, but rather the files inside. I do think giving the option to rename each data selection would be useful.

This being said, I do like the layout and the way each piece of data that possible for download is displayed. I don't know if this is possible, but the slow double-click on Mac or Windows for file/directory renaming would be awesome here (I can attach a video if this is unclear). The current display could be kept in this case, and the user could selectively chose which files they wanted to rename without a bunch of input text-boxes.

@gibsongreen
Copy link
Contributor

One idea I can think of is, let's say the user extracts a spectrum. In the Export plugin, in the Data Selection, this would auto populate as it should. The extracted spectrum could be the default selection made by the plugin, since this is new data comparatively to the ERR or SCI data which they had from the original file loaded.

Another idea is, let's say the user extracts a spectrum and creates a moment map (or any combination of multiple pieces of new data). Instead of toggling the multi-select button on, if the default idea previously is desirable, the default in this case would use multi-select, and default to the selection of both the spectrum and the moment map for download.

@Jenneh idea for the button at the end of the plugins for exporting. If the user had generated data that they wanted to export, and then clicked this button, the default irregardless could have this data pre-selected in the Export plugin. More importantly, when the this button is clicked, the Export plot could open, and the data generated could be temporarily highlighted in the Export Plugin, to inform them of where their desired export data is within the lists of selections.

@pllim
Copy link
Contributor

pllim commented Feb 23, 2024

FWIW, this is how Ginga does it. I implemented it for TEL based on their functional requirements:

https://ginga.readthedocs.io/en/stable/manual/plugins_global/saveimage.html

Which is used to extend it to TEL specific functionality here:

https://wss-tools.readthedocs.io/en/latest/wss_tools/quip/savequip.html

Not the best design but in case you find it useful as a reference.

@kecnry kecnry force-pushed the export-plugin-design branch 2 times, most recently from d09af68 to a2eea20 Compare February 27, 2024 18:40
@kecnry kecnry changed the title Export plugin design Export plugin (deprecates/replaces "Export Plot") Mar 4, 2024
@kecnry kecnry force-pushed the export-plugin-design branch 2 times, most recently from f951b52 to 6681c0e Compare March 6, 2024 19:51
@kecnry kecnry marked this pull request as ready for review March 6, 2024 20:07
@kecnry
Copy link
Member Author

kecnry commented Mar 6, 2024

This is now ready for review as a replacement of the previous "Export Plot" plugin, aiming to provide all the same functionality as previously, but with the design shown above and hidden behind the feature flags to move forward.

Regarding the design review comments above, we are planning to improve the shortcut icon in the viewers and add the capability to directly scroll to the plugin when clicked. We can also consider buttons in places where creating an instance of this plugin, prepopulated, in a popout or overlay would be beneficial.

@kecnry kecnry force-pushed the export-plugin-design branch from 6681c0e to 06f4e72 Compare March 8, 2024 14:06
@cshanahan1
Copy link
Contributor

cshanahan1 commented Mar 8, 2024

thanks for clarifying. i was assuming this PR was adding all 'savable' things to the export plugin (like subsets) without the ability to actually save them yet so i was expecting them to be there.

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.

LGTM, all my concerns were either already existing in main or things that will be addressed as we build out the functionality of the currently hidden stuff.

@@ -326,8 +326,8 @@ have valid flux units. For 3D data, the current :ref:`slice` is used.

.. _cubeviz-export-plot:
Copy link
Contributor

Choose a reason for hiding this comment

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

should this reference name be changed from _cubeviz-export-plot to cubeviz-export (in this and in {otherconfigs)/plugins.rst?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure, I didn't want to break existing RTD links so left them alone, but maybe we should put in redirects.... @pllim - what's the best path forward here?

Copy link
Contributor

Choose a reason for hiding this comment

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

The reference name isn't user facing, I would just leave it so existing intersphinx won't break.

@cshanahan1
Copy link
Contributor

theres a small reference to the old plugin name in test_template_mixin on line 113

@cshanahan1
Copy link
Contributor

Otherwise, my concerns are all out of scope so i will approve.

(One more out-of-scope just so I don't forget - right now when you select a viewer to export, a dropdown to select file type appears. This will happen for the other items once the export is actually implemented, correct?)

@kecnry
Copy link
Member Author

kecnry commented Mar 8, 2024

One more out-of-scope just so I don't forget - right now when you select a viewer to export, a dropdown to select file type appears. This will happen for the other items once the export is actually implemented, correct?

Yes, we'll need to implement per-"section" format dropdowns, as needed.

Please make sure all of your out-of-scope comments are covered by issues/tickets!

@kecnry
Copy link
Member Author

kecnry commented Mar 8, 2024

(rebasing will be a bit complicated but just copying the changes in export_plot.py from #2715 to export.py - I'll work on that and fix the reference before merging)

@kecnry kecnry force-pushed the export-plugin-design branch from da472de to 3f5a0d7 Compare March 8, 2024 20:19
@kecnry kecnry merged commit 0655d4a into spacetelescope:main Mar 8, 2024
14 of 16 checks passed
@kecnry kecnry deleted the export-plugin-design branch March 8, 2024 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cubeviz documentation Explanation of code and concepts imviz mosviz plugin Label for plugins common to multiple configurations Ready for final review specviz specviz2d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants