BUG: fix figure layout in case the long axis is vertical

[neutrinoceros]

PR Summary

fix #3632
Running the script therein I get:

I expect this to fail CI because we surely have a couple images whose size is going to change, and unfortunately I don't think our testing infra is currently able to manage this properly (see #3602)

[neutrinoceros]

I modified the testing framework to make image comparison possible even when shapes diverge

[neutrinoceros]

Adding this to the 4.0.2 meta issue as a candidate for the bug fix release, but it's not 100% clear that it should be considered a bug fix so I'd like this to be discussed in the review.

[neutrinoceros]

Got a handful of expected failures, exclusively for images that are targeted by the patch, so I'm opening for review.

[neutrinoceros]

I'm not convinced this plays correctly with user-specified aspect. Switching to draft for now

[neutrinoceros]

Fixed the issue with user-defined aspect, I believe the result is now what users would expect: the size of the long axis stays consistent

import yt

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

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

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

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

[neutrinoceros]

I think I got this to a stable state. Now it seems rather obvious that the code could be simplified/clarified, so I'm keeping the draft status, and I'll make a final push on this when I have time to focus on it again.

[neutrinoceros]

I realized that my modified testing framework produces nice reports but will upload polluted artifacts that are not suited to update the answer store.
It needs more work on this point too

[neutrinoceros]

I'm going to try to validate this by updating the answer store early. I'll do this in the least disruptive way possible by pushing a branch directly to the mother repo and check it out here so I don't need to iterate with reviewers to see wether my modified framework works as intended.

[neutrinoceros]

It's alive ! now let's try to simplify the logic

[neutrinoceros]

I think more and more that this should be considered a bug fix since I'm fulfilling the original intent of this code.

[neutrinoceros]

@Xarthisius, if you get a chance, could you review the changes I did to the testing framework here please ?

[chrishavlin]

So after a lot of thought, I don't think this is quite the right approach. Consider the final two images you plotted:

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

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

The standard definition of an aspect ratio is width / height. But those final two images break that definition: in the first, you're setting an aspect ratio < 1 and getting an image where width > height and in the second the opposite. I think we should keep the standard definition.

The base problem you're trying to address is that small aspect ratio plots end up too tall, right? So why not adjust the default figure width to a smaller value if aspect ratio < 1 so that the height ends up more reasonable?

[neutrinoceros]

The standard definition of an aspect ratio is width / height. But those final two images break that definition: in the first, you're setting an aspect ratio < 1 and getting an image where width > height and in the second the opposite. I think we should keep the standard definition.

So I hear what you're saying, unfortunately the aspect argument, as it's actually used internally, is rather the ratio of L0 / vertical space over L0 / horizontal space where L0 is any reference length in physical units, and vertical/horizontal space are measured in pixels (or any screen/paper relevant unit). I believe my current implementation is faithful to this pre-existing design.

Also note that the baseline example (not using the aspect argument actually has an aspect of 1, but it's not a square, because the simulation domain isn't a square either. This is a bit tricky and perhaps exposing the _unit_aspect attribute as simply aspect was a mistake, but that shipped has sailed and I'm trying to work with the existing API.

[matthewturk]

@chrishavlin do you have thoughts on the current state?

[chrishavlin]

Ya, I've looked at this a few times the past couple days and I don't feel great about it, but I guess this PR isn't really the place to address the confusing way we handle aspect ratios. I think my main issue is that aspect ends up being passed to imshow so we should be conforming to the standard definition of the aspect ratio expected by imshow regardless of how it's currently used internally by us. But ya, I suppose this PR isn't the spot to fix that (if it is even something that should be addressed).

[neutrinoceros]

ok then, thanks for approving. I'll update this with an actual companion PR to the answer store

[neutrinoceros]

Here's the companion PR yt-project/answer-store#29

[neutrinoceros]

Update: I've validated the companion PR to the answer store by checking it in on this side, so I believe it's good to go now.
Then I'll update the present PR again to point to the perennial branch of the answer store, and bump into answers for Jenkins as well so we should get CI back to green.

[neutrinoceros]

thanks @cphyc for merging the companion PR. I bumped everywhere needed and I now expect CI to turn green