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

pnnx convert onnx depth2space, add pixelshuffle pixelunshuffle test #5967

Merged
merged 3 commits into from
Apr 1, 2025

Conversation

nihui
Copy link
Member

@nihui nihui commented Apr 1, 2025

No description provided.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds new tests for pixel shuffle and pixel unshuffle operations on ONNX conversion using pnnx. The changes introduce unit tests for both the nn module and functional APIs related to pixel shuffle/unshuffle operations, ensuring conversion correctness.

Reviewed Changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated 4 comments.

File Description
tools/pnnx/tests/onnx/test_nn_PixelUnshuffle.py Added test for nn.PixelUnshuffle conversion
tools/pnnx/tests/onnx/test_nn_PixelShuffle.py Added test for nn.PixelShuffle conversion
tools/pnnx/tests/onnx/test_F_pixel_unshuffle.py Added test for F.pixel_unshuffle conversion
tools/pnnx/tests/onnx/test_F_pixel_shuffle.py Added test for F.pixel_shuffle conversion
Files not reviewed (2)
  • tools/pnnx/src/pass_level2/F_pixel_shuffle.cpp: Language not supported
  • tools/pnnx/tests/onnx/CMakeLists.txt: Language not supported

Comment on lines +57 to +58
return torch.equal(a0, b0) and torch.equal(a1, b1)

Copy link
Preview

Copilot AI Apr 1, 2025

Choose a reason for hiding this comment

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

Using torch.equal for comparing floating-point tensors may lead to false negatives due to minor numerical differences. Consider switching to torch.allclose with appropriate tolerance.

Suggested change
return torch.equal(a0, b0) and torch.equal(a1, b1)
return torch.allclose(a0, b0) and torch.allclose(a1, b1)

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Comment on lines +57 to +58
return torch.equal(a0, b0) and torch.equal(a1, b1)

Copy link
Preview

Copilot AI Apr 1, 2025

Choose a reason for hiding this comment

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

Using torch.equal for comparing floating-point tensors can be too strict after ONNX conversion. It may be more robust to use torch.allclose with a tolerance to account for potential numerical variability.

Suggested change
return torch.equal(a0, b0) and torch.equal(a1, b1)
return torch.allclose(a0, b0, atol=1e-6) and torch.allclose(a1, b1, atol=1e-6)

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Comment on lines +48 to +49
return torch.equal(a, b)

Copy link
Preview

Copilot AI Apr 1, 2025

Choose a reason for hiding this comment

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

Comparing tensors with torch.equal may not accommodate minor numerical discrepancies post-conversion. Consider using torch.allclose with a defined tolerance for a more reliable test.

Suggested change
return torch.equal(a, b)
return torch.allclose(a, b)

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Comment on lines +48 to +49
return torch.equal(a, b)

Copy link
Preview

Copilot AI Apr 1, 2025

Choose a reason for hiding this comment

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

Using torch.equal for floating-point tensor comparison might be overly strict. It is advisable to use torch.allclose with an appropriate tolerance to ensure robust test verification.

Suggested change
return torch.equal(a, b)
return torch.allclose(a, b, atol=1e-08)

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

@nihui nihui merged commit f3651ce into Tencent:master Apr 1, 2025
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant