-
Notifications
You must be signed in to change notification settings - Fork 76
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
Button to export Cubeviz movie #2264
Changes from all commits
0ed62d9
e3cc8f7
0c0ebda
805b646
28784b6
10447f0
6569c1a
e1f469f
e5a1e11
8825308
7c84445
ab50909
71be125
927b423
2e607a4
002c270
f33b44a
5b22463
49effa9
4976cfe
ad07f02
a7f6f9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,20 +137,29 @@ def _write_moment_to_fits(self, overwrite=False, *args): | |
if self.moment is None or not self.filename: # pragma: no cover | ||
return | ||
|
||
path = Path(self.filename).resolve() | ||
if path.exists(): | ||
# Make sure file does not end up in weird places in standalone mode. | ||
path = os.path.dirname(self.filename) | ||
if path and not os.path.exists(path): | ||
raise ValueError(f"Invalid path={path}") | ||
elif (not path or path.startswith("..")) and os.environ.get("JDAVIZ_START_DIR", ""): # noqa: E501 # pragma: no cover | ||
filename = Path(os.environ["JDAVIZ_START_DIR"]) / self.filename | ||
else: | ||
filename = Path(self.filename).resolve() | ||
Comment on lines
+140
to
+147
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd REALLY like to architect a pathlib-only solution here and avoid using I'm generally a stickler for path handling and I admit I was tempted to leave a "changes requested" review here, but I'll bite my tongue and allow myself to get overruled here since we're at the end of the sprint There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I have the same sentiment but refactoring moment map is out of scope here. Moment map is already like that when I joined the project, so not sure what was the original intention of the non-pathlib design. Keep in mind also that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless you mean specifically that you do not like |
||
|
||
if filename.exists(): | ||
if overwrite: | ||
# Try to delete the file | ||
path.unlink() | ||
if path.exists(): | ||
filename.unlink() | ||
if filename.exists(): | ||
# Warn the user if the file still exists | ||
raise FileExistsError(f"Unable to delete {path}. Check user permissions.") | ||
raise FileExistsError(f"Unable to delete {filename}. Check user permissions.") | ||
else: | ||
self.overwrite_warn = True | ||
return | ||
|
||
self.moment.write(str(path)) | ||
filename = str(filename) | ||
self.moment.write(filename) | ||
|
||
# Let the user know where we saved the file. | ||
self.hub.broadcast(SnackbarMessage( | ||
f"Moment map saved to {os.path.abspath(self.filename)}", sender=self, color="success")) | ||
f"Moment map saved to {os.path.abspath(filename)}", sender=self, color="success")) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
import os | ||
|
||
import pytest | ||
|
||
from jdaviz.configs.default.plugins.export_plot.export_plot import HAS_OPENCV | ||
|
||
|
||
# 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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the same problem I encountered in #929 |
||
# @pytest.mark.skipif(not HAS_OPENCV, reason="opencv-python is not installed") | ||
def test_export_movie(cubeviz_helper, spectrum1d_cube, tmp_path): | ||
orig_path = os.getcwd() | ||
os.chdir(tmp_path) | ||
try: | ||
cubeviz_helper.load_data(spectrum1d_cube, data_label="test") | ||
plugin = cubeviz_helper.plugins["Export Plot"] | ||
assert plugin.i_start == 0 | ||
assert plugin.i_end == 1 | ||
assert plugin.movie_filename == "mymovie.mp4" | ||
|
||
plugin._obj.vue_save_movie("mp4") | ||
assert os.path.isfile("mymovie.mp4"), tmp_path | ||
finally: | ||
os.chdir(orig_path) | ||
|
||
|
||
@pytest.mark.skipif(HAS_OPENCV, reason="opencv-python is installed") | ||
def test_no_opencv(cubeviz_helper, spectrum1d_cube): | ||
cubeviz_helper.load_data(spectrum1d_cube, data_label="test") | ||
plugin = cubeviz_helper.plugins["Export Plot"] | ||
assert plugin._obj.movie_msg != "" | ||
with pytest.raises(ImportError, match="Please install opencv-python"): | ||
plugin.save_movie() | ||
|
||
|
||
@pytest.mark.skipif(not HAS_OPENCV, reason="opencv-python is not installed") | ||
def test_export_movie_not_cubeviz(imviz_helper): | ||
plugin = imviz_helper.plugins["Export Plot"] | ||
|
||
with pytest.raises(NotImplementedError, match="save_movie is not available for config"): | ||
plugin._obj.save_movie() | ||
|
||
# Also not available via plugin public API. | ||
with pytest.raises(AttributeError): | ||
plugin.save_movie() | ||
|
||
|
||
@pytest.mark.skipif(not HAS_OPENCV, reason="opencv-python is not installed") | ||
def test_export_movie_cubeviz_exceptions(cubeviz_helper, spectrum1d_cube): | ||
cubeviz_helper.load_data(spectrum1d_cube, data_label="test") | ||
cubeviz_helper.default_viewer.shape = (100, 100) | ||
cubeviz_helper.app.get_viewer("uncert-viewer").shape = (100, 100) | ||
plugin = cubeviz_helper.plugins["Export Plot"] | ||
assert plugin._obj.movie_msg == "" | ||
assert plugin.i_start == 0 | ||
assert plugin.i_end == 1 | ||
assert plugin.movie_filename == "mymovie.mp4" | ||
|
||
with pytest.raises(NotImplementedError, match="filetype"): | ||
plugin.save_movie(filetype="gif") | ||
|
||
with pytest.raises(NotImplementedError, match="filetype"): | ||
plugin.save_movie(filename="mymovie.gif", filetype=None) | ||
|
||
with pytest.raises(ValueError, match="No frames to write"): | ||
plugin.save_movie(i_start=0, i_end=0) | ||
|
||
with pytest.raises(ValueError, match="Invalid frame rate"): | ||
plugin.save_movie(fps=0) | ||
|
||
plugin.movie_filename = "fake_path/mymovie.mp4" | ||
with pytest.raises(ValueError, match="Invalid path"): | ||
plugin.save_movie() | ||
|
||
plugin.movie_filename = "mymovie.mp4" | ||
plugin.viewer = 'spectrum-viewer' | ||
with pytest.raises(TypeError, match=r"Movie for.*is not supported"): | ||
plugin.save_movie() | ||
|
||
plugin.movie_filename = "" | ||
plugin.viewer = 'uncert-viewer' | ||
with pytest.raises(ValueError, match="Invalid filename"): | ||
plugin.save_movie() | ||
|
||
|
||
@pytest.mark.skipif(not HAS_OPENCV, reason="opencv-python is not installed") | ||
def test_export_movie_cubeviz_empty(cubeviz_helper): | ||
plugin = cubeviz_helper.plugins["Export Plot"] | ||
assert plugin.i_start == 0 | ||
assert plugin.i_end == 0 | ||
|
||
with pytest.raises(ValueError, match="Selected viewer has no display shape"): | ||
plugin.save_movie(i_start=0, i_end=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camipacifici , is a note here sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤔 ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, you want all errors go to this traitlet thingy instead of snackbar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First thing first... do you not see this when opencv-python is not installed and you click "export to MP4"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this? Note that I cannot insert HTML links into the text, so I can just do plain text.