3
\$\begingroup\$

I have the following function:

def get_resize_dimensions(new_width: int, new_height: int, width: int, height: int) -> tuple[int, int]:
    """Return the new width and height of an image, resized to the new width and height but maintaining the aspect
    ratio. It will shorten along the edge that leaves the other edge less than or equal to the desired height. This
    means that we can keep the aspect ratio and pad a shorter edge with white space if needed."""
    h_gt = new_height < height
    w_gt = new_width < width
    h_lt = new_height > height
    w_lt = new_width > width
    if (h_lt and w_lt) or (h_gt and w_gt):
        # (new_height, new_width < desired) or (new_height, new_width > desired)
        if new_width / width > new_height / height:
            return int(new_height / height * width), new_height
        return new_width, int(new_width / width * height)
    elif h_lt and w_gt:  # new_height < desired, new_width > desired
        return int(new_height / height * width), new_height
    elif h_gt and w_lt:  # new_height > desired, new_width < desired
        return new_width, int(new_height / height * height)
    elif new_height == height and new_width == width:  # new_height, new_width = desired
        return new_width, new_height
    else:  # height == width
        _min = min(new_width, new_height)  # choose the smaller of the two
        return _min, _min

It is described how it works in the code. It will take in an image of dimension \$(x,y)\$ and the user wants the image to be dimension \$(u,v)\$. The function will calculate \$(u*,v*)\$ such that one of these cases is true:

  • \$u* = u, v* \leq v\$
  • \$u* \leq u, v* = v\$

The key thing to note here is neither of \$u*\$ or \$v*\$ should be greater than \$u\$\ or \$v\$, respectively, and at least one of \$u*\$ equals \$u\$, or \$v*\$ equals \$v\$.

This will then allow the user to pad whitespace along the shorter edge to make the image of size \$(u,v)\$.

However, I feel that the code looks messy, is hard to follow and probably not a very effective or efficient way of doing it. I haven't really tested it yet so I am not 100% certain it is correct, although I am 99% sure.

Is there any known algorithms to sort this or a better way I could do this?

Note: There are no bounds on the input image dimensions, it could literally be anything.

Edit: Some test cases: I am not 100% sure if my function works for all of these, but these are the expected outputs.

I think this covers every case.

Input                | Output
-----------------------------------
(1500,1000,900,1400) |(643,1000)
(1500,1000,1000,900) |(1111,1000)
(1500,1000,1600,1100)|(1455,1000)
(1500,1000,1900,1100)|(1500,868)
(1500,1000,1400,1100)|(1273,1000)
(1500,1000,1600,900) |(1500,844)
(1500,1000,1500,1000)|(1500,1000)
(1500,1000,1200,1200)|(1000,1000)
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Can the inputs be negative? (I suggest ignoring this, but if it's a legitimate use-case then you'll need to be quite picky about some of the math.) \$\endgroup\$ Apr 22, 2022 at 16:23
  • \$\begingroup\$ @ShapeOfMatter no they shouldn't be as the inputs are from a PIL Image object. \$\endgroup\$
    – GAP2002
    Apr 22, 2022 at 16:51
  • \$\begingroup\$ I'm not convinced those tests are correct. Youre passing w and h in as new_width and new_height to the function, but printing them out as if they're width and height. \$\endgroup\$
    – Josiah
    Apr 22, 2022 at 17:28
  • \$\begingroup\$ Ah my bad, I actually calculated them by hand so they are correct just not visually correct. \$\endgroup\$
    – GAP2002
    Apr 22, 2022 at 17:46

3 Answers 3

3
\$\begingroup\$

Focussing on the code as you've written it, there are a few things that alarmed me at a first pass.

  • Separately checking A>B and B>A tends to be a bit of a red flag because of the risk that the equality case gets dropped. It can be needed, but I'd always try to partition into just two cases if possible.
  • As you note, that's quite a lot of if statements, and quite a lot of conditions on each one. If the complexity can be reduced that's good. If not, I'd look for ways to at restructure the conditions so that they follow a more consistent pattern. This is disputable, but I'd probably go so far as to add some duplicate code to make it more readable, at least as a precursor to perhaps simplifying it down.
  • I don't want to nitpick naming too much, but I'd favour similar levels of verbosity for similar parameters. I find new_width and width just a little bit jarring. If the latter were original_width for example, it would be smoother. Normally I'd complain about h_gt but in this structure it works OK.
  • int will always round down. That may be what you want, but you may want to consider expressly rounding to the nearest integer instead. That is most likely to be relevant when the target aspect ratio actually matches the input aspect ratio, but floating point errors mess things up for you.

Meanwhile a couple of points that I very much do like:

  • I appreciate the use of type annotations.
  • The docstring is clear in describing what it does and why.

In terms of simplifying things, as already mentioned by ShapeOfMatter, most of the cases can be reduced to a single check. Personally I find it more intuitive to compare aspect ratios rather than lengths, using something of the shape:

if new_width / new_height > width / height:
    # The target aspect ratio is wider than the original
    return round(new_height * width / height), new_height
else:
    # The target aspect ratio is the same as or taller than the original
    return new_width, round(new_width * height / width)
\$\endgroup\$
5
  • \$\begingroup\$ Regarding, missing the equality case this was semi-intentional as it allowed the else at the bottom. I believe this gives incorrect outputs: Width: 1500, Height: 1200, New Width: 1500, New Height: 1000, Output: (1500, 1250) Width: 1400, Height: 1000, New Width: 1500, New Height: 1000, Output: (1500, 1400) Width: 900, Height: 900, New Width: 1500, New Height: 1000, Output: (1500, 1000) Width: 1600, Height: 1600, New Width: 1500, New Height: 1000, Output: (1500, 1000) ^ These are all wrong \$\endgroup\$
    – GAP2002
    Apr 22, 2022 at 17:03
  • \$\begingroup\$ Sorry, yes. There's something backwards there \$\endgroup\$
    – Josiah
    Apr 22, 2022 at 17:13
  • \$\begingroup\$ I have added some test cases to the question, hopefully, they help! \$\endgroup\$
    – GAP2002
    Apr 22, 2022 at 17:13
  • \$\begingroup\$ I'd editted my suggestion in response to your first set of tests, but now it seems you've changed the requirements. I'm afraid I'll have to leave fixing it up to the exact pattern you have in mind "as an exercise" as they say. \$\endgroup\$
    – Josiah
    Apr 22, 2022 at 17:37
  • \$\begingroup\$ Aside from a few errors I made writing the tests, it looks as though this now works. Thanks! \$\endgroup\$
    – GAP2002
    Apr 22, 2022 at 17:51
4
\$\begingroup\$

It seems like this function probably shouldn't exist and you're doing too much work. You say that you have a PIL image. A trivial search for "pillow resize preserving aspect" yields Image.thumbnail:

This method modifies the image to contain a thumbnail version of itself, no larger than the given size. This method calculates an appropriate thumbnail size to preserve the aspect of the image

So it's very likely that you should delete all of your code and call that instead. If you don't want to use it for whatever reason, you can look to its implementation for inspiration:

        def round_aspect(number, key):
            return max(min(math.floor(number), math.ceil(number), key=key), 1)

        # preserve aspect ratio
        aspect = self.width / self.height
        if x / y >= aspect:
            x = round_aspect(y * aspect, key=lambda n: abs(aspect - n / y))
        else:
            y = round_aspect(
                x / aspect, key=lambda n: 0 if n == 0 else abs(aspect - x / n)
            )
        size = (x, y)
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Ooo. I actually checked to see if PIL resize had a relevant option, but I ignored thumbnail because I wasn't sure (and still am not) that it offered the same breadth of functionality (algorithms, color transforms, etc). +1 for pulling the source code of the existing solution! But they're handling the rounding in a very different way, which may or may not be good for OP. \$\endgroup\$ Apr 23, 2022 at 13:15
  • \$\begingroup\$ @ShapeOfMatter What other functionality? It is confusingly named and does the transformation in-place, which is not always convenient, but you are left with the good ol' PIL Image, so... Same functionality as in the rest of the package. \$\endgroup\$
    – Lodinn
    Apr 23, 2022 at 20:09
  • \$\begingroup\$ Yeah, I think this probably is the best solution, although I did have in mind that it might not just be for PIL images. I did see this mentioned in other places, however, the name thumbnail suggests reduced-size versions of pictures or videos, not reduced, increased, the same etc. \$\endgroup\$
    – GAP2002
    Apr 23, 2022 at 20:22
3
\$\begingroup\$

Assuming there are no sneaky problems we're overlooking with the rounding-to-int step, there are actually only two choices we need to consider:

  • \$u'=u,v'\leq v\$
  • \$v'=v,u'\leq u\$

So a single if...else should suffice.
Off the cuff:

def get_resize_dimensions(new_width: int,
                          new_height: int,
                          width: int,
                          height: int
                         ) -> tuple[int, int]:
    """
    Get the maximum dimensions
    having the aspect ratio of `width and height`
    and fitting inside `new_width` and `new_height`.
    
    Returns:
    tuple[int, int]: (width, height)
    """
    ratio = width / height
    guess_width = int(new_height * ratio)
    return (guess_width, new_height)
           if guess_width <= new_width
           else (new_width, int(new_width / ratio))

I'm not 100% certain of the interchangeability of a//b vs int(a/b). (This is moot the way I did the math.)

I'm also not happy with all the variable names. Maybe {width|height}_{target|existing}?

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Note that // is integer division, which is a very large rounding error to introduce into an intermediate result. A 640x480 image would report a ratio of 1. Even worse, a 480x640 image would report a ratio of 0! \$\endgroup\$
    – Josiah
    Apr 22, 2022 at 16:32
  • \$\begingroup\$ {facepalm} Of course integer division isn't appropriate for the ratio; sorry! \$\endgroup\$ Apr 22, 2022 at 16:34
  • \$\begingroup\$ I agree variable names could be improved and I definitely will change these! However, I think this fails on these tests: Width: 1500, Height: 1200, New Width: 1500, New Height: 1000, Output: (1500, 1000) Width: 1400, Height: 1000, New Width: 1500, New Height: 1000, Output: (1400, 933) Width: 900, Height: 900, New Width: 1500, New Height: 1000, Output: (900, 600) Width: 1600, Height: 1600, New Width: 1500, New Height: 1000, Output: (1600, 1066) \$\endgroup\$
    – GAP2002
    Apr 22, 2022 at 17:08
  • \$\begingroup\$ I have added some test cases to the question, hopefully, they help! \$\endgroup\$
    – GAP2002
    Apr 22, 2022 at 17:13
  • 1
    \$\begingroup\$ Probably I made a math error. Let me know if you find it :) \$\endgroup\$ Apr 22, 2022 at 17:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.