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

BUG: figures with height > width are too tall #3632

Closed
neutrinoceros opened this issue Oct 31, 2021 · 2 comments · Fixed by #3633
Closed

BUG: figures with height > width are too tall #3632

neutrinoceros opened this issue Oct 31, 2021 · 2 comments · Fixed by #3633
Labels
bug enhancement Making something better viz: 2D
Milestone

Comments

@neutrinoceros
Copy link
Member

Bug report

Bug summary

related to #3631
Currently when plotting a region that happens to be broader in "height" (vertical axis) than in "width" (horizontal axis), the output figure may be very tall and not as readable as the standard square figure produced by yt.

Code for reproduction

ds = yt.load_sample("KelvinHelmholtz")

slc = yt.SlicePlot(ds, "z", "density")
slc.save("/tmp/rect_1.png")

Actual outcome
zy

Expected outcome

I'd except a smaller figure, that would fit my screen while staying readable, on par with what I obtain after flipping axes:

vals = ds.coordinates.y_axis["z"], ds.coordinates.x_axis["z"]
ds.coordinates.x_axis["z"], ds.coordinates.y_axis["z"] = vals
ds.coordinates.x_axis[2], ds.coordinates.y_axis[2] = vals
slc = yt.SlicePlot(ds, "z", "density")
slc.save("/tmp/rect_2.png")

yz

I think the root cause is trivially that the figure width is hardcoded while the aspect ratio is constant.
I would make more sense to hardcode the width of the largest dimension, wether it happens to be the width or the height of the figure, and it would be backwards compatible in cases where it doesn't matter.

@neutrinoceros neutrinoceros added enhancement Making something better viz: 2D labels Oct 31, 2021
@neutrinoceros
Copy link
Member Author

After looking into the code I think it may even be considered a bug, because the following comment shows intention of implementing the behaviour I expect, not the one we actual see

def _get_best_layout(self):
# Ensure the figure size along the long axis is always equal to _figure_size

@neutrinoceros
Copy link
Member Author

I think I got half the fix with this patch:

--- a/yt/visualization/base_plot_types.py
+++ b/yt/visualization/base_plot_types.py
@@ -343,12 +343,12 @@ class ImagePlotMPL(PlotMPL):
     def _get_best_layout(self):

         # Ensure the figure size along the long axis is always equal to _figure_size
-        if is_sequence(self._figure_size):
-            x_fig_size = self._figure_size[0]
-            y_fig_size = self._figure_size[1]
-        else:
-            x_fig_size = self._figure_size
-            y_fig_size = self._figure_size / self._aspect
+        if not is_sequence(self._figure_size):
+            self._figure_size = (self._figure_size, self._figure_size / self._aspect)
+            if self._aspect < 1:
+                self._figure_size = tuple(reversed(self._figure_size))
+
+        x_fig_size, y_fig_size = self._figure_size

         if hasattr(self, "_unit_aspect"):
             y_fig_size = y_fig_size * self._unit_aspect

running the script from the OP again yields

zy
(the axis-flipped version is unchanged)
So the elements of the figure have the desired size, but the window size is now too wide somehow.

@neutrinoceros neutrinoceros changed the title ENH: figures with height > width are too tall BUG: figures with height > width are too tall Nov 9, 2021
@neutrinoceros neutrinoceros added this to the 4.0.2 milestone Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug enhancement Making something better viz: 2D
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant