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
Adding testcases for visualization.image_writer #1842
Conversation
0a4f66d
to
862c42a
Compare
862c42a
to
e1c7242
Compare
yt/visualization/image_writer.py
Outdated
if len(bitmap_array.shape) != 3 or bitmap_array.shape[-1] not in (3,4): | ||
raise RuntimeError | ||
if len(bitmap_array.shape) != 3 or bitmap_array.shape[-1] not in (3, 4): | ||
raise YTException(message="Expecting image array of shape (N,M,3)" |
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 just use RuntimeError
instead of YTException
here.
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.
fixed
yt/visualization/image_writer.py
Outdated
@@ -295,8 +299,8 @@ def strip_colormap_data(fn = "color_map_data.py", | |||
f.write("from numpy import array\n") | |||
f.write("color_map_luts = {}\n\n\n") | |||
if cmaps is None: cmaps = rcm.ColorMaps | |||
if isinstance(cmaps, string_types): cmaps = [cmaps] |
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.
carriage return after the colon
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.
fixed
if suffix == ".png": | ||
canvas = FigureCanvasAgg(fig) | ||
elif suffix == ".pdf": | ||
if suffix == ".pdf": |
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 you restore the logic that was originally here? We want the warning message about the unknown suffix I think.
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.
get_image_suffix(filename)
returns ['.png', '.eps', '.ps', '.pdf', '']
thus, it will never execute the original else
block.
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.
Ah, fair enough
import tempfile | ||
import unittest | ||
|
||
from yt.mods import write_projection |
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.
import this from yt.visualization.image_writer
.
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.
fixed
# Test remaining parameters of write_projection | ||
write_projection(image, "test_2", xlabel="x-axis", ylabel="y-axis") | ||
write_projection(image, "test_3.pdf", xlabel="x-axis", ylabel="y-axis") | ||
write_projection(image, "test_4.eps", xlabel="x-axis", ylabel="y-axis") |
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 you test to make sure these images are actually written to disk and are in the correct format? See the assert_fname
function in yt.testing
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.
fixed
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.
Updated as per the first code review.
yt/visualization/image_writer.py
Outdated
if len(bitmap_array.shape) != 3 or bitmap_array.shape[-1] not in (3,4): | ||
raise RuntimeError | ||
if len(bitmap_array.shape) != 3 or bitmap_array.shape[-1] not in (3, 4): | ||
raise YTException(message="Expecting image array of shape (N,M,3)" |
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.
fixed
yt/visualization/image_writer.py
Outdated
@@ -295,8 +299,8 @@ def strip_colormap_data(fn = "color_map_data.py", | |||
f.write("from numpy import array\n") | |||
f.write("color_map_luts = {}\n\n\n") | |||
if cmaps is None: cmaps = rcm.ColorMaps | |||
if isinstance(cmaps, string_types): cmaps = [cmaps] |
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.
fixed
import tempfile | ||
import unittest | ||
|
||
from yt.mods import write_projection |
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.
fixed
# Test remaining parameters of write_projection | ||
write_projection(image, "test_2", xlabel="x-axis", ylabel="y-axis") | ||
write_projection(image, "test_3.pdf", xlabel="x-axis", ylabel="y-axis") | ||
write_projection(image, "test_4.eps", xlabel="x-axis", ylabel="y-axis") |
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.
fixed
@yt-fido test this please |
1 similar comment
@yt-fido test this please |
PR Summary
Adding testcases for
visualization.image_writer
PR Checklist
Adding Reviewers
@ngoldbaum @Xarthisius @colinmarc