Skip to content

testintel: add missing test for AvgDown3D.forward (created by linxwang) - #1083

Closed
jiagaoxiang wants to merge 3 commits into
AMD-AGI:mainfrom
jiagaoxiang:testintel/test/cand-9cf47afdeacb6b6d0378129eb4bf01c2
Closed

testintel: add missing test for AvgDown3D.forward (created by linxwang)#1083
jiagaoxiang wants to merge 3 commits into
AMD-AGI:mainfrom
jiagaoxiang:testintel/test/cand-9cf47afdeacb6b6d0378129eb4bf01c2

Conversation

@jiagaoxiang

Copy link
Copy Markdown
Collaborator

This PR was created by linxwang via Test Gap Resolver.

No existing test in tests/unit_tests/backends/diffusion (or anywhere else in the repo) instantiates AvgDown3D or imports vae2_2.py at all; the pad->view->permute->view->view->mean sequence that implements grouped spatiotemporal average-downsampling, including the temporal padding branch (pad_t>0) and the group_size averaging when in_channelsfactor != out_channelsgroup_size trivially, is completely unexercised.

This PR adds a focused unit test for AvgDown3D.forward; it does not change production code.

Candidate cand:9cf47afdeacb6b6d0378129eb4bf01c2 at 2f01706.
Copilot AI lite review requested due to automatic review settings September 3, 2026 08:38
@WangLingxun

Copy link
Copy Markdown
Collaborator

Please revise the spatial/channel test before marking this PR ready. test_forward_downsamples_spatial_and_channel_dims currently asserts only the output shape, so it would not catch an incorrect permute order or group_size channel mapping. Use a deterministic position-distinguishable input (for example, torch.arange) and compare the complete output against an independent numerical reference for factor_s=2 with in_channels != out_channels. The reference should verify both spatial block grouping and the contiguous channel/group mapping, rather than reusing AvgDown3D internals. Keep the existing temporal front-padding and non-divisible-constructor coverage. This is regression coverage; no current production bug is asserted.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

One of the new tests validates only output shape (not values), which leaves the core downsampling/reshaping semantics unverified.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a new unit test module to exercise the previously untested AvgDown3D.forward path in the Wan VAE 2.2 implementation, focusing on temporal padding and grouped channel averaging behavior.

Changes:

  • Introduces tests/unit_tests/backends/diffusion/test_wan_vae_avgdown3d.py covering: (1) grouped averaging without padding, (2) temporal left-padding to a multiple of factor_t, and (3) initialization-time channel-factor compatibility assertions.
  • Adds a basic shape check for spatial downsampling + channel regrouping.
File summaries
File Description
tests/unit_tests/backends/diffusion/test_wan_vae_avgdown3d.py Adds new unit tests for AvgDown3D.forward, including temporal padding and grouped averaging scenarios.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +39 to +48
def test_forward_downsamples_spatial_and_channel_dims():
factor_t, factor_s = 1, 2
in_channels, out_channels = 2, 4
module = AvgDown3D(in_channels=in_channels, out_channels=out_channels, factor_t=factor_t, factor_s=factor_s)

b, t, h, w = 1, 2, 4, 4
x = torch.randn(b, in_channels, t, h, w)
out = module(x)

assert out.shape == (b, out_channels, t, h // factor_s, w // factor_s)
…rence

Address review: compare full output against a hand-computed reference for
factor_s=2 with in_channels != out_channels (deterministic torch.arange
input), verifying spatial block grouping and channel/group mapping instead
of only asserting output shape.
Copilot AI review requested due to automatic review settings September 3, 2026 08:42
@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

TestIntel PR Steward: Addressed @WangLingxun's review comment in 4d37455.

test_forward_downsamples_spatial_and_channel_dims now uses factor_t=1, factor_s=2, in_channels=2, out_channels=4 with a torch.arange (position-distinguishable) input, and asserts the full output against an independent, hand-computed reference (no calls into AvgDown3D's own pad/view/permute/view/view/mean sequence). The reference derives, for every input element, which (output_channel, spatial_block) slot it lands in from factor_t/factor_s/group_size alone, accumulates the sums, and checks each group has exactly group_size contributors before comparing with torch.testing.assert_close. This exercises both the spatial block grouping (h/w downsample) and the contiguous channel/group mapping, and would fail on an incorrect permute order or group_size mapping.

The existing temporal front-padding and non-divisible-constructor tests are unchanged, as requested.

I verified the reference formula against the actual module logic with a standalone reshape/permute simulation before pushing, so this should be correct, but I don't have a torch environment in this sandbox to run pytest directly — please let CI (or a manual run) confirm.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The added tests accurately match the implementation’s padding and grouping logic and introduce no production-code risk.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

pre-commit's black hook wraps the AvgDown3D(...) constructor call in
test_forward_downsamples_spatial_and_channel_dims onto multiple lines
(line was too long). No behavior change.
Copilot AI review requested due to automatic review settings September 3, 2026 08:46
@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

TestIntel PR Steward: CI's code-lint (3.12) job failed on 323b8616 because black (run via pre-commit) wanted to reflow one line in test_forward_downsamples_spatial_and_channel_dims (the AvgDown3D(...) constructor call was too long) — this is the same line touched by the fix for @WangLingxun's / Copilot's shape-only-assertion finding. I applied exactly the formatting pre-commit proposed (wrapped the constructor call onto multiple lines, no logic change) in 323b8616c024cc38b6a19845e5a566b139640dae and pushed it to this branch. Expecting code-lint (3.12) to go green on the next run.

@WangLingxun / Copilot: the underlying test-content change (full-tensor comparison against an independent reference instead of shape-only) is unchanged from my earlier reply — only formatting moved.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change is test-only and the added assertions correctly match the current AvgDown3D implementation semantics (padding, reshaping/permute order, and grouping).

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

Superseded by same-repo PR #1096 (head AMD-AGI:testintel/test/cand-9cf47afdeacb6b6d0378129eb4bf01c2) so Primus-CI-TAS receives Docker Hub credentials and can run torch unit tests. Closing this fork PR.

@jiagaoxiang jiagaoxiang closed this Sep 4, 2026
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.

3 participants