fix: CodeRabbit auto-fixes for PR #1396 - #1401
Merged
Merged
Conversation
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
Deployment failed with the following error: Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit |
Contributor
Author
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
4 tasks done
ooples
added a commit
that referenced
this pull request
May 20, 2026
…dictor — fixes 2× output-length shape mismatch (#1396) * fix(#1304 c6): drop Dropout from OccupancyNN defaults; fix memorization invariant PR #1290 CI Cluster 6 #1304: OccupancyNeuralNetworkTests.LossStrictlyDecreasesOnMemorizationTask was reported to be fixed by PR #1329's BatchNorm→LayerNorm swap, but the test was still red on master with loss step 1=0.6936, step 100=0.7032 (slightly INCREASING) — model stuck at the BCE-ln(2) baseline through 100 gradient steps. ## Root cause PR #1329 fixed the BN-at-batch-1 degeneracy (σ²=0 → y=β collapses the gradient through normalization) but the *Dropout layer*'s memorization-blocking effect was not addressed. The default Occupancy layer stack was: Dense(64)+ReLU → LayerNorm → Dropout(0.3) → Dense(32)+ReLU → LayerNorm → Dropout(0.2) → Dense(16)+ReLU → Dense(out)+Sigmoid Under the model-family LossStrictlyDecreasesOnMemorizationTask invariant — train the SAME (x, target) pair for 100 iterations and assert loss strictly decreases — every forward pass under Dropout sees a DIFFERENT random sub-network (~56% of hidden units active = 0.7 × 0.8). On a 3 → 64 → 32 → 16 → 1 MLP (~2k params), the per-step mask randomness injects more variance than the gradient can subtract over 100 steps, leaving loss flat or slightly RISING at the BCE-ln(2) baseline. ## Fix Remove Dropout from both `CreateDefaultOccupancyLayers` and `CreateDefaultOccupancyTemporalLayers` in `LayerHelper<T>`. At this network size Dropout adds no useful regularization (the model has fewer params than typical sensor batches have rows); callers who genuinely need regularization on a larger Occupancy MLP can pass an explicit architecture with their preferred Dropout rate. ## Verification $ dotnet test --filter "FullyQualifiedName~OccupancyNeuralNetworkTests" Passed! - Failed: 0, Passed: 21, Skipped: 0, Total: 21 All 21 OccupancyNN tests pass (was 1 failing). The 4 remaining #1304 tests post-fix: - SimCSETests.TrainingError_ShouldNotExceedTestError PASS (was passing already on current master) - SimCSETests.Training_ShouldChangeParameters PASS (was passing already on current master) - DenseNetNetworkTests.MoreData_ShouldNotDegrade Adam-overshoot divergence (200-iter loss > 50-iter loss); separate follow-up issue - NEATTests.Training_ShouldReduceLoss timeout (perf gap, similar to #1390); separate follow-up issue Closes #1304 partially. DenseNet + NEAT follow-ups tracked separately. * fix(#1305 cluster-6): port patchify/unpatchify to FluxDoubleStreamPredictor — fixes 2× output-length shape mismatch PR #1290 CI Cluster 6 #1305: Flux2SchnellModelTests.ScaledInput_ShouldChangeOutput was failing on master with: System.InvalidOperationException : PredictNoise output length (32768) does not match the latent/sample length (16384). Check that the noise predictor's output shape matches the input. Identical class of bug the MMDiTXNoisePredictor fix in #1224 Cluster F (ControlNetSD3) closed for the SD3 MMDiT-X variant. Same root cause; same fix shape. ## Root cause `FluxDoubleStreamPredictor.PredictNoise` ran the Dense block stack directly on the rank-4 spatial tensor `[B, C, H, W]`: ```csharp var x = _patchEmbed.Forward(noisySample); foreach (var block in _doubleBlocks) x = block.Forward(x); foreach (var block in _singleBlocks) x = block.Forward(x); return _finalLayer.Forward(x); ``` DenseLayer applied along the last axis projects `W → patchDim` and emits `[B, C, H, patchDim]`. For the FLUX default `[1, 16, 32, 32]` with `patchSize=2` (so `patchDim = inputChannels * 4 = 64`), the output is `[1, 16, 32, 64] = 32768` elements — exactly 2× the latent at 16384, which is what `DiffusionModelBase.Generate`'s shape check catches. The DiT-style architecture FLUX implements (Esser et al. 2024 §3, Black Forest Labs 2024) requires patchify before the block stack and unpatchify after: ``` [B, C, H, W] → Patchify → [B, (H/P)·(W/P), C·P²] → DenseBlocks → Unpatchify → [B, C, H, W] ``` The MMDiTXNoisePredictor fix in `src/Diffusion/NoisePredictors/MMDiTXNoisePredictor.cs:165-283` already implements this exact pattern. Port it. ## Fix Apply the same Patchify/Unpatchify + rank normalization scaffolding from MMDiTXNoisePredictor to `FluxDoubleStreamPredictor.PredictNoise`: 1. Normalize rank-3 [C,H,W] → rank-4 [1,C,H,W] for unbatched test inputs. 2. Patchify [B,C,H,W] → [B, (H/P)·(W/P), C·P²] using the standard `rearrange("b c (h p1) (w p2) → b (h w) (c p1 p2)")` pattern. 3. Run `_patchEmbed → _doubleBlocks → _singleBlocks → _finalLayer` on the token tensor as the implementation intended. 4. Unpatchify back to [B,C,H,W]. 5. Re-strip the batch dim if the input was unbatched. ## Verification $ dotnet test --filter "FullyQualifiedName=AiDotNet.Tests.ModelFamilyTests.Diffusion.Flux2SchnellModelTests.ScaledInput_ShouldChangeOutput" --framework net10.0 Passed! - Failed: 0, Passed: 1, Skipped: 0, Total: 1, Duration: 55 s The test passes in 55 s under isolation (the 120 s test envelope). Under parallel xUnit execution Flux can hit the timeout because the foundation-scale UNet at [1, 16, 32, 32] competes for CPU with adjacent diffusion tests — that's a separate perf-gap issue, not a shape bug. ## Adjacent state for #1305 - `Flux2SchnellModelTests.ScaledInput_ShouldChangeOutput` — PASS in isolation after this fix - `VideoCrafterModelTests.ScaledInput_ShouldChangeOutput` — already PASS on master (intervening work) - `ConsistencyModelTests.ScaledInput_ShouldChangeOutput` — still times out at 120 s on the [1, 4, 64, 64] UNet at foundation scale; perf-gap issue, sibling to #1394 (ResNet/VGG ImageNet-scale perf) Closes #1305 partially (the actual shape-contract bug). Foundation-scale diffusion perf gap is a separate follow-up. * fix(pr1396-review): extract patchsize to class-level const addresses coderabbit nit on pr #1396: line 110 had int patchdim = _inputchannels * 4; // 2x2 patches while line 185 in predictnoise had int patchdim = channels * patchsize * patchsize; where patchsize was a local const inside predictnoise. drift-prone since the magic 4 in initializelayers must stay in sync with the patchsize used in patchify/unpatchify. fix: promote patchsize to a class-level const near the other fields, update initializelayers to derive patchdim from it, and remove the local const in predictnoise. one definition, no drift. build passes. * fix: apply CodeRabbit auto-fixes (#1401) Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai> --------- Co-authored-by: franklinic <franklin@ivorycloud.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This stacked PR contains CodeRabbit auto-fixes for #1396.
Files modified:
src/Diffusion/NoisePredictors/FluxDoubleStreamPredictor.cs