Skip to content

Conversation

@swesterfeld
Copy link
Collaborator

@swesterfeld swesterfeld commented Oct 22, 2025

I've seen that the aspect ratio of the j90randomsizej80 test is not correct. randomsize.inc contains as extremes (with ~1000 images):

281x891
1274x403

which are both not in 16:9...9:16. With the fix applied, the extremes are

358x634
1335x752

which are what I'd expect it to be.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Greptile Summary

This PR fixes a mathematical bug in the test suite's random image resolution generator. The previous implementation would randomly select one dimension (either width or height) within a scaled range and derive the other, which failed to maintain the intended aspect ratio constraints between9:16 and 16:9. The new implementation correctly calculates both dimensions from the target pixel area using the mathematical relationship where width = sqrt(area * ratio) and height = sqrt(area / ratio), where ratio is the aspect ratio (width/height). This ensures test images maintain realistic proportions within the specified bounds. The fix impacts the tests/gen-tests-mk script which generates test configurations for validating watermark robustness across various image geometries. The PR description demonstrates the fix effectiveness by showing that previously generated extremes (281x891and 1274x403) violated aspect ratio constraints, while corrected extremes (358x634 and 608x344) properly fall within the 9:16 to 16:9 range.

Changed Files
Filename Score Overview
tests/gen-tests-mk 5/5 Fixed aspect ratio calculation to properly derive both width and height from pixel area and aspect ratio using square root relationships

Confidence score: 5/5

  • This PR is safe to merge with minimal risk; the fix corrects a clear mathematical error in test generation logic.
  • Score reflects a straightforward, mathematically sound correction to a single calculation with demonstrable improvement in output; no production code paths are affected, only test generation.
  • No files require special attention; the change is isolated to test infrastructure with clear before/after validation provided in the PR description.

Sequence Diagram

sequenceDiagram
    participant User
    participant main
    participant setup_icdf
    participant generate_resolutions
    participant random
    participant CubicSpline
    participant filesystem

    User->>main: "Execute gen-tests-mk with args"
    main->>random: "Set seed"
    main->>setup_icdf: "Create ICDF from resolution table"
    setup_icdf-->>main: "Return (min, max, pdf, cdf, icdf)"
    main->>generate_resolutions: "Generate resolutions with min_ratio/max_ratio"
    generate_resolutions->>random: "Generate random probabilities"
    generate_resolutions->>CubicSpline: "Interpolate resolutions from ICDF"
    loop For each resolution
        generate_resolutions->>random: "Sample aspect ratio in [9/16, 16/9]"
        generate_resolutions->>generate_resolutions: "Calculate width = sqrt(pixels * ratio)"
        generate_resolutions->>generate_resolutions: "Calculate height = sqrt(pixels / ratio)"
    end
    generate_resolutions-->>main: "Return list of (width, height) tuples"
    main->>filesystem: "Create output directories"
    loop For each input image
        main->>filesystem: "Symlink image with target size"
    end
    main->>filesystem: "Write randomsize.inc with RANDOMSIZE assignments"
Loading

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

randreso = np.random.randint (round (reso * min_ratio), round (reso * max_ratio) + 1)
resolutions2d.append ((randreso, round (reso * reso / randreso)))
total_pixels = reso * reso
ratio = random.uniform (min_ratio, max_ratio)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider using np.random.uniform instead of random.uniform for consistency with numpy ecosystem (line 93 uses Python's random, but other randomness uses numpy)

Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/gen-tests-mk
Line: 105:105

Comment:
**style:** Consider using `np.random.uniform` instead of `random.uniform` for consistency with numpy ecosystem (line 93 uses Python's random, but other randomness uses numpy)

How can I resolve this? If you propose a fix, please make it concise.

@tim-janik tim-janik closed this Oct 22, 2025
@tim-janik tim-janik merged commit 2b853e1 into trunk Oct 22, 2025
2 checks passed
@tim-janik tim-janik deleted the fix-random-size-aspect-ratio branch October 22, 2025 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants