Button to export Cubeviz movie #2264

pllim · 2023-06-23T22:54:01Z

Description

This pull request is to add functionality to export Cubeviz slices into a movie file.

Doc: https://jdaviz--2264.org.readthedocs.build/en/2264/cubeviz/plugins.html#movie

Example video generated from flux viewer: https://www.youtube.com/watch?v=KticlwspGV4

Fixes #2245 and fixes #2271

TODO

Expose FPS to user.
Baby Shark roundtrip in concept notebook.

After merge

Open follow-up issue about GIF support. See Cubeviz movie the sequel: The GIF that keeps giving #2281
Open follow-up issue about Button to export Cubeviz movie #2264 (comment) , see Standalone app: Save movie makes axes invisible in light mode #2282

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

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

pllim · 2023-06-27T21:59:47Z

jdaviz/configs/cubeviz/plugins/tests/test_export_plots.py

+
+
+# TODO: Remove skip when https://github.com/bqplot/bqplot/pull/1397/files#r726500097 is resolved.
+@pytest.mark.skip(reason="Cannot test due to async JS callback")


This is the same problem I encountered in #929

pllim · 2023-06-27T22:02:04Z

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

+        if filetype != "mp4":
+            raise NotImplementedError(f"filetype={filetype} not supported")


To also support GIF, yet another dependency (imageio) needs to be introduced. I'd rather not have to tackle that in this PR. I think it would be cleaner as a follow-up PR if we really want that.

codecov · 2023-06-27T22:13:58Z

Codecov Report

Patch coverage: 65.57% and project coverage change: -0.28 ⚠️

Comparison is base (b6feaa6) 91.34% compared to head (a7f6f9a) 91.07%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2264      +/-   ##
==========================================
- Coverage   91.34%   91.07%   -0.28%     
==========================================
  Files         150      151       +1     
  Lines       16848    17024     +176     
==========================================
+ Hits        15390    15504     +114     
- Misses       1458     1520      +62

Impacted Files	Coverage Δ
...configs/default/plugins/export_plot/export_plot.py	`53.73% <55.33%> (+5.34%)`	⬆️
...configs/cubeviz/plugins/tests/test_export_plots.py	`75.75% <75.75%> (ø)`
...configs/cubeviz/plugins/moment_maps/moment_maps.py	`95.12% <90.00%> (+0.25%)`	⬆️
...eviz/plugins/moment_maps/tests/test_moment_maps.py	`100.00% <100.00%> (ø)`
jdaviz/configs/cubeviz/plugins/slice/slice.py	`93.91% <100.00%> (ø)`

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

pllim · 2023-06-27T22:35:24Z

devdeps failure is unrelated; I reported it upstream at https://github.com/newville/asteval/issues/120

camipacifici · 2023-06-28T15:14:14Z

Looks very nice, thank you!
A few thoughts:

There should be a note at the top of the "Export video" to mention the needed package and a link to its installation instructions.
Ignore this! it would be useful to be able to select the viewer (I created a smoothed cube and opened it in the uncertainty viewer and to save that one out I had to load it in the flux viewer first. I would be easier to just select the viewer I want)
In standalone, I could not find the movie. Maybe a note to tell the user that they have to set the full path (rev: I actually tried, I misspelled the path, so the path did not exist, but it said it created the movie anyway at /private/var/folders/js/kn36084x6k988161dxy2fwlw0001hl/T/tmp4icsvifz/my-misspelled-path. If it created something, I have no idea where it is ahah)? Or a file picker would be even better.
I would like to have the option to set the delay between each frame. If I play the movie on my machine, I can play it at a different speed, but if I send this to a journal, I do not think they can set the speed arbitrarily.

Thank you!

camipacifici · 2023-06-28T15:16:11Z

Ah! Revision: it followed the viewer I set for export plot! It is ok then, maybe we just need Jenn to make the UI more intuitive.

camipacifici · 2023-06-28T15:27:52Z

User says it should also show the axis, please.

pllim · 2023-06-28T15:39:28Z

option to set the delay between each frame

I can present it as frame rate in FPS or absolute delay in milliseconds between frames. I think the former is more common in video software and gaming, but either is possible. Please pick one.

User says it should also show the axis, please.

Not sure what this means. It already does. Please see https://www.youtube.com/watch?v=KticlwspGV4 (also linked in original post above). (UPDATE: Cami said she could not see axis in video standalone mode but axis shows in the app. Things appear normal in notebook though.)

More comment: Needs a space between filename and the rest of sentence in success snackbar message.

I'll see if I can address the rest in a follow up commit. Thanks for the quick response!

Note to self: Look at Moment Map plugin for outfile support. Baby Shark roundtrip in concept notebook.

camipacifici · 2023-06-28T15:58:23Z

Frame rate is good!

pllim · 2023-06-28T20:24:16Z

User says it should also show the axis, please.

I don't know why but turns out when in standalone app, the axes are written out as black text on black background, rendering them invisible. If you use the "save PNG" feature to snap one frame, you can see that the axes text is black and transparent background. Is it following my OS theme (which is dark) instead of voila theme (light mode when this happened). If I start the standalone app using --theme=dark, I can see the axes: https://www.youtube.com/watch?v=_qwVtpycGUo

Is the solution to tell bqplot to not do transparent background, but rather detect the theme and assign black/white background appropriately? Not sure. Maybe the code is at https://github.com/bqplot/bqplot/blob/29105c8056262445a2474d1252db9ab77c024b32/js/src/Figure.ts#L1170-L1194

camipacifici · 2023-06-28T21:02:48Z

to provide full path in standalone app

it should give an error if the path does not exist.

pllim · 2023-06-28T22:13:40Z

docs/cubeviz/plugins.rst

+
+.. note::
+
+    For MPEG-4, this feature needs ``opencv-python`` to be installed;


@camipacifici , is a note here sufficient?

Can we detect this in the plugin init, set a traitlet, and swap out the UI with a message instead? Right now the UI shows the option, but clicking "export movie" just doesn't do anything (although looking at the code - I'm not sure why the snackbar error message never showed 🤔 ).

Huh, really? I saw a red snackbar asking me to install opencv-python when I ran it in a new env and forgot to install first.

So, you want all errors go to this traitlet thingy instead of snackbar?

I'd like to see the note in the plugin itself at a minimum, together with the link to the docs.
I did see the red snackbar when I first used it without having installed the additional package.

No, but I think we can detect that the functionality is completely disabled because of a missing dependency and warn the user about that in advance. If there is a snackbar though, then that is probably sufficient for now and I can look into seeing why it didn't show on my install (or maybe I just missed it).

First thing first... do you not see this when opencv-python is not installed and you click "export to MP4"?

How about this? Note that I cannot insert HTML links into the text, so I can just do plain text.

pllim · 2023-06-29T00:42:13Z

I think I have addressed the comments as my ability allows. I have noted follow-up issues to open for the things I cannot fix here. See "after merge" in original post. Thanks!

as promised.

[ci skip] [rtd skip] Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

and add comment to Slice.

but it only shows when activated

since disabling is not obvious enough, also moar tooltip

viewer in Cubeviz [ci skip] [rtd skip] Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>

and remove unnecessary frontend check now that Ricky's suggestion is accepted.

On the screen, it is not the computed background color that is seen by a user. Since DOM elements can be transparent, the background color might be one of the parent elements. This is why we need to traverse the DOM tree to find the background color of the first non-transparent parent element. A more sophisticated solution would be to also use blending when there is a non-fully transparent background color. I think that might be a bit overkill for now. See also spacetelescope/jdaviz#2264 (comment)

* feat: pyinstaller * codesign osx * debug: try without codesign * does not need arguments * use hooks * also download the app * use branch of pyinstaller * better hooks * pin to 5.11 Otherwise we need to do at least: + rm -rf standalone/dist/jdaviz.app/Contents/MacOS/jedi/third_party/typeshed/stdlib/ + rm -rf standalone/dist/jdaviz.app/Contents/MacOS/**/*.dist-info * fix: maintain symlinks by zipping, upload-artifact does not support it * GHA logic? * code sign on gha * run notary tool on gha * fix: redo codesign after modifications * fix: reorder zipping and notary step * remove invalid symlink * make sure the program executes * fix path of entitlements file * add comment for hint with the osx fix * gpt assisted way of running the log tool on failure * fix syntax error * fix path * comments for the future * fix uuid parsing and re-zip the app after notary step * fix: use ditto instead of zip from https://developer.apple.com/forums/thread/116831 zip might cause issues with utf8 or metadata. * comments for the future * fix uuid for notary step * upload different artifact for osx * cleanup spec file and hooks, no more __pycache__ files should be included * ci: do not cancel on failure * fix: mistune 3.0 needs this * make dmg instead of zip for osx * BUG: Fix mouseover behavior in Cubeviz spectrum viewer when spatial subset is present. Co-authored-by: Duy Nguyen <duytnguyendtn.open@gmail.com> * Remove change log from #2258 because the bug only affects unreleased code [ci skip] [rtd skip] * Deprecate get_subsets_from_viewer * Missing region index * Fix subset args * Remove get_data_from_viewer from imviz viewer tests * Remove get_data_from_viewer from mosviz data loading test and sub hardcoded viewer ref names * Mosviz test update image truth class * Fix incorrect viewer ref * Set Mos2Dviewer data statistic to None by default * Non-existent data check * Properly deprecate getters * Properly check for valueError on non-existent label * Update Specviz get_data_from_viewer test * Rename "subset_to_apply" to "spectral_subset * Codestyle * Fix docs wording Co-authored-by: Jesse Averbukh <javerbukh@gmail.com> * Catch missed code, fix bug * Retain Mosviz get_data behavior and fix change log * Undo bad diff * MNT: Add .mailmap so git shortlog -es gives sane listing. * DOC: Add warning about surface brightness in Simple Aperture Photometry plugin (#2261) * DOC: Add warning about surface brightness in Simple Aperture Photometry plugin. I am beginning to think the Simple in plugin name no longer applies. * DOC: Improve verbiage. Co-authored-by: Camilla Pacifici <camilla.pacifici@gmail.com> --------- Co-authored-by: Camilla Pacifici <camilla.pacifici@gmail.com> * FEAT: Annulus draw tool for Imviz (#2240) * FEAT: Annulus draw tool for Imviz. TODO: Need to fix icon. TST: Add tests. [ci skip] [rtd skip] * Pull in #2204 into this PR and bump upstream pins * Changed function name upstream in glue-astronomy during review process * Proper annulus icon from J. Kotler Co-authored-by: Jennifer Kotler <jennlkotler@gmail.com> * Avoid error traceback with bad annulus radii * Disable recentering for annulus * Fix test failure because not sure why spectral region is using Imviz centering method but okay. --------- Co-authored-by: Jennifer Kotler <jennlkotler@gmail.com> Co-authored-by: Ricky O'Steen <rosteen@stsci.edu> * MNT: Temporarily pin voila<0.5 (#2269) * MNT: Temp pin voila<0.5 because voila-template is incompatible, see #2268 * TST: Disable voila dev in test matrix * TST: Ignore DeprecationWarning from asteval (#2274) * TST: Ignore DeprecationWarning from asteval. For example https://github.com/newville/asteval/issues/120 * Also ignore FutureWarning from asteval * Add doc for windows latency issue * Deprecate load_spectrum * Update tests and docs from load_spectrum * Docstring suggestions Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> * Changelog * Changelog * Button to export Cubeviz movie (#2264) * WIP: Backend API to write movie file but got stuck with bqplot not cooperating after first frame. [ci skip] [rtd skip] * fix: bqplot stuck after the first frame of the movie We have to run the loop outside the main thread; otherwise, the processing of messages from the frontend is blocked, causing the message with the first image to never be received. The "save_image" method can only save the next image after the previous image is received. * Fix typo, this works now from the API. [ci skip] [rtd skip] * Add frontend and tests * Add user doc * DOC: Add note about standalone app saving file into weird places. * Address some review comments * Expose FPS and fix test * DOC: Baby Shark roundtrip as promised. * DOC: Ellie said more shark! * Vue.js style improvements [ci skip] [rtd skip] Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> * Display pls install msg in plugin * Address review comments * Fix test * Fix path resolution in standalone app * Improve frontend validation and access. Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> * Disable video for spectrum-viewer in GUI and add comment to Slice. * Add tooltip to kill switch but it only shows when activated * Rename kill with something less scary * Hide stop button since disabling is not obvious enough, also moar tooltip * Disable the whole movie menu for spectrum viewer in Cubeviz [ci skip] [rtd skip] Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com> * Fix RTD warnings about invalid prop and remove unnecessary frontend check now that Ricky's suggestion is accepted. --------- Co-authored-by: Mario Buikhuizen <mariobuikhuizen@gmail.com> Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com> * Add functional but ugly launcher * Use launcher notebook if neither config nor filepath is specified * Codestyle * Add margins on left and right to avoid cutoff in notebook * Support launcher from cli * Codestyle * Remove URI from path text until implemented * Fix standalone bug * Specify --layout= as new required cli syntax * Remove unused import Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> * Update readme to show required layout flag * Changelog * Changelog Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> * Changelog * Update .github/workflows/standalone.yml Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> * Update .github/workflows/standalone.yml * Update to use config launcher * Moved changelog entry to new section --------- Co-authored-by: Duy Nguyen <duytnguyendtn.open@gmail.com> Co-authored-by: Pey Lian Lim <2090236+pllim@users.noreply.github.com> Co-authored-by: Duy Tuong Nguyen <dtn5ah@virginia.edu> Co-authored-by: Jesse Averbukh <javerbukh@gmail.com> Co-authored-by: Camilla Pacifici <camilla.pacifici@gmail.com> Co-authored-by: Jennifer Kotler <jennlkotler@gmail.com> Co-authored-by: Ricky O'Steen <rosteen@stsci.edu> Co-authored-by: Mario Buikhuizen <mariobuikhuizen@gmail.com> Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>

On the screen, it is not the computed background color that is seen by a user. Since DOM elements can be transparent, the background color might be one of the parent elements. This is why we need to traverse the DOM tree to find the background color of the first non-transparent parent element. A more sophisticated solution would be to also use blending when there is a non-fully transparent background color. I think that might be a bit overkill for now. See also spacetelescope/jdaviz#2264 (comment)

) On the screen, it is not the computed background color that is seen by a user. Since DOM elements can be transparent, the background color might be one of the parent elements. This is why we need to traverse the DOM tree to find the background color of the first non-transparent parent element. A more sophisticated solution would be to also use blending when there is a non-fully transparent background color. I think that might be a bit overkill for now. See also spacetelescope/jdaviz#2264 (comment) Co-authored-by: Maarten Breddels <maartenbreddels@gmail.com>

pllim added cubeviz feature Feature request labels Jun 23, 2023

pllim added this to the 3.6 milestone Jun 23, 2023

github-actions bot added documentation Explanation of code and concepts testing labels Jun 23, 2023

pllim commented Jun 23, 2023

View reviewed changes

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

pllim force-pushed the you-gotta-movie-it-movie-it branch from 99b6839 to 52cd264 Compare June 27, 2023 19:02

pllim commented Jun 27, 2023

View reviewed changes

pllim marked this pull request as ready for review June 27, 2023 22:36

pllim requested review from duytnguyendtn, rosteen, javerbukh, kecnry, bmorris3, haticekaratay and cshanahan1 as code owners June 27, 2023 22:36

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

pllim commented Jun 28, 2023

View reviewed changes

pllim and others added 15 commits July 3, 2023 10:12

Expose FPS and fix test

e1f469f

DOC: Baby Shark roundtrip

e5a1e11

as promised.

DOC: Ellie said more shark!

8825308

Vue.js style improvements

7c84445

[ci skip] [rtd skip] Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

Display pls install msg in plugin

ab50909

Address review comments

71be125

Fix test

927b423

Fix path resolution in standalone app

2e607a4

Improve frontend validation and access.

002c270

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

Disable video for spectrum-viewer in GUI

f33b44a

and add comment to Slice.

Add tooltip to kill switch

5b22463

but it only shows when activated

Rename kill with something less scary

49effa9

Hide stop button

4976cfe

since disabling is not obvious enough, also moar tooltip

Disable the whole movie menu for spectrum

ad07f02

viewer in Cubeviz [ci skip] [rtd skip] Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>

Fix RTD warnings about invalid prop

a7f6f9a

and remove unnecessary frontend check now that Ricky's suggestion is accepted.

pllim force-pushed the you-gotta-movie-it-movie-it branch from a0bd852 to a7f6f9a Compare July 3, 2023 14:14

pllim merged commit 24ff314 into spacetelescope:main Jul 3, 2023

pllim deleted the you-gotta-movie-it-movie-it branch July 3, 2023 14:43

This was referenced Jul 3, 2023

Cubeviz movie the sequel: The GIF that keeps giving #2281

Open

Standalone app: Save movie makes axes invisible in light mode #2282

Open

maartenbreddels mentioned this pull request Jul 4, 2023

fix: background color of figure was not computed properly bqplot/bqplot#1603

Merged

kecnry mentioned this pull request Jul 6, 2023

update style guide for consistency between plugins #2286

Merged

9 tasks

martinRenou mentioned this pull request Jul 11, 2023

Backport #1603 fix: background color of figure was not computed properly (#1603) bqplot/bqplot#1606

Merged

kecnry mentioned this pull request Sep 11, 2023

API access (hidden) to disable cubeviz movie UI #2440

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Button to export Cubeviz movie #2264

Button to export Cubeviz movie #2264

pllim commented Jun 23, 2023 •

edited

Loading

pllim Jun 27, 2023

pllim Jun 27, 2023

codecov bot commented Jun 27, 2023 •

edited

Loading

pllim commented Jun 27, 2023

camipacifici commented Jun 28, 2023 •

edited

Loading

camipacifici commented Jun 28, 2023

camipacifici commented Jun 28, 2023

pllim commented Jun 28, 2023 •

edited

Loading

camipacifici commented Jun 28, 2023

This comment was marked as outdated.

pllim commented Jun 28, 2023 •

edited

Loading

This comment was marked as resolved.

camipacifici commented Jun 28, 2023

This comment was marked as outdated.

pllim Jun 28, 2023

kecnry Jun 29, 2023

pllim Jun 29, 2023

pllim Jun 29, 2023

camipacifici Jun 29, 2023

kecnry Jun 29, 2023

pllim Jun 29, 2023

pllim Jun 29, 2023

pllim commented Jun 29, 2023 •

edited

Loading



		# TODO: Remove skip when https://github.com/bqplot/bqplot/pull/1397/files#r726500097 is resolved.
		@pytest.mark.skip(reason="Cannot test due to async JS callback")

		if filetype != "mp4":
		raise NotImplementedError(f"filetype={filetype} not supported")


		.. note::

		For MPEG-4, this feature needs ``opencv-python`` to be installed;

Button to export Cubeviz movie #2264

Button to export Cubeviz movie #2264

Conversation

pllim commented Jun 23, 2023 • edited Loading

Description

TODO

After merge

Change log entry

Checklist for package maintainer(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 27, 2023 • edited Loading

Codecov Report

pllim commented Jun 27, 2023

camipacifici commented Jun 28, 2023 • edited Loading

camipacifici commented Jun 28, 2023

camipacifici commented Jun 28, 2023

pllim commented Jun 28, 2023 • edited Loading

camipacifici commented Jun 28, 2023

This comment was marked as outdated.

pllim commented Jun 28, 2023 • edited Loading

This comment was marked as resolved.

camipacifici commented Jun 28, 2023

This comment was marked as outdated.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Jun 29, 2023 • edited Loading

pllim commented Jun 23, 2023 •

edited

Loading

codecov bot commented Jun 27, 2023 •

edited

Loading

camipacifici commented Jun 28, 2023 •

edited

Loading

pllim commented Jun 28, 2023 •

edited

Loading

pllim commented Jun 28, 2023 •

edited

Loading

pllim commented Jun 29, 2023 •

edited

Loading