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

fix: SRCNN impl and tests #26

Merged
merged 1 commit into from
Nov 24, 2024
Merged

fix: SRCNN impl and tests #26

merged 1 commit into from
Nov 24, 2024

Conversation

Tohrusky
Copy link
Member

@Tohrusky Tohrusky commented Nov 24, 2024

Summary by Sourcery

Fix the SRCNN implementation by correcting the RGB to YUV conversion condition and add tests for utility functions to ensure proper error handling and functionality.

Bug Fixes:

  • Fix the condition in the SRCNN architecture to correctly handle RGB to YUV conversion only when the input tensor has 3 channels.

Tests:

  • Add new tests for utility functions including device configuration and color conversion functions, ensuring they handle errors and perform conversions correctly.

Copy link

sourcery-ai bot commented Nov 24, 2024

Reviewer's Guide by Sourcery

This PR fixes the SRCNN implementation by adjusting the color space conversion logic and adds comprehensive tests for color conversion utilities. The main changes include a condition update in the SRCNN forward pass and new test coverage for RGB-YUV conversion.

Class diagram for new test utilities

classDiagram
    class TestUtilities {
        + test_device(): None
        + test_color(): None
    }
    class ColorConversion {
        + rgb_to_yuv(input)
        + yuv_to_rgb(input)
    }
    TestUtilities --> ColorConversion : uses
    note for TestUtilities "New tests for color conversion utilities"
Loading

File-Level Changes

Change Details Files
Modified SRCNN forward pass color space conversion logic
  • Updated condition to check both number of channels and input tensor channels
  • Added additional check for x.size(1) == 3 to ensure proper RGB input handling
ccrestoration/arch/srcnn_arch.py
Added comprehensive test coverage for color conversion utilities
  • Added type checking tests for rgb_to_yuv and yuv_to_rgb functions
  • Added tensor dimension validation tests
  • Implemented end-to-end color conversion test with image similarity check
tests/test_util.py
Updated coverage configuration
  • Added device utility module to coverage omit list
pyproject.toml

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@Tohrusky Tohrusky changed the title fix: srcnn impl and tests fix: SRCNN impl and tests Nov 24, 2024
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @Tohrusky - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟡 Testing: 2 issues found
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +12 to +13
def test_device() -> None:
print(DEFAULT_DEVICE)
Copy link

Choose a reason for hiding this comment

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

issue (testing): Test function test_device only prints the device without any assertions

This test doesn't verify any behavior. Consider adding assertions to check if DEFAULT_DEVICE is set correctly or has expected properties.

Comment on lines +16 to +25
def test_color() -> None:
with pytest.raises(TypeError):
rgb_to_yuv(1)
with pytest.raises(TypeError):
yuv_to_rgb(1)

with pytest.raises(ValueError):
rgb_to_yuv(torch.zeros(1, 1))
with pytest.raises(ValueError):
yuv_to_rgb(torch.zeros(1, 1))
Copy link

Choose a reason for hiding this comment

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

suggestion (testing): Test function test_color could benefit from parametrization and more specific assertions

Consider splitting this test into multiple test cases: one for error conditions, one for RGB->YUV conversion, and one for YUV->RGB conversion. Also, consider adding assertions about the specific values or ranges in the YUV color space to ensure the conversion is correct.

@pytest.mark.parametrize("invalid_input", [1, "string", [1, 2, 3]])
def test_color_type_errors(invalid_input) -> None:
    with pytest.raises(TypeError):
        rgb_to_yuv(invalid_input)
    with pytest.raises(TypeError):
        yuv_to_rgb(invalid_input)

@pytest.mark.parametrize("invalid_shape", [torch.zeros(1, 1), torch.zeros(2, 3, 4)])
def test_color_value_errors(invalid_shape) -> None:
    with pytest.raises(ValueError):
        rgb_to_yuv(invalid_shape)
    with pytest.raises(ValueError):
        yuv_to_rgb(invalid_shape)

def test_color_conversion() -> None:
    img = load_image()
    img_rgb = cv2.cvtColor(img, cv2.COLOR_BGR2RGB)
    img_tensor = transforms.ToTensor()(img_rgb).unsqueeze(0).to("cpu")

    yuv = rgb_to_yuv(img_tensor)
    assert yuv.shape == img_tensor.shape
    assert (yuv[:, 0] >= -1).all() and (yuv[:, 0] <= 1).all()  # Y channel bounds

    rgb = yuv_to_rgb(yuv)
    rgb_np = (rgb.squeeze(0).permute(1, 2, 0).cpu().numpy() * 255).clip(0, 255).astype("uint8")
    rgb_np = cv2.cvtColor(rgb_np, cv2.COLOR_RGB2BGR)

    assert calculate_image_similarity(rgb_np, img)

@Tohrusky Tohrusky merged commit 23ece6a into TensoRaws:main Nov 24, 2024
12 checks passed
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.

1 participant