Fix the PPLCNetV3 detection backbone channel count - #446
Conversation
|
Warning Review limit reachedNext included review available in 15 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change removes redundant PPLCNetV3 channel scaling and adds tests for recognition, detection, and EMA initialization behavior. ChangesPPLCNetV3 backbone validation
EMA callback initialization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The detection backbone now uses the actual layer output widths, preventing channel mismatches during detection model construction. Recognition behavior remains covered, and the change is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #446 +/- ##
=========================================
+ Coverage 0 94.61% +94.61%
=========================================
Files 0 290 +290
Lines 0 15868 +15868
=========================================
+ Hits 0 15013 +15013
- Misses 0 855 +855
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
0fb0481 to
2e18780
Compare
`LCNetV3Layer` scales its `out_channels` in the constructor. The backbone scaled that value a second time. The detection convolutions then expected 224 channels, but the blocks gave 240. A config with `use_detection_backbone: true` crashed for this reason. Use the block widths directly. Add a unit test for both backbone modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QA7RcK3rsE5mHX7J6X1Vxh
`EMACallback.ema` raises before `on_fit_start` builds the model. No test covered that path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QA7RcK3rsE5mHX7J6X1Vxh
2e18780 to
8033ee5
Compare
Purpose
PPLCNetV3crashes when a configuration setsuse_detection_backboneto
true. The detection branch has never worked. This PR corrects thechannel count and adds the tests that the branch never had.
The PR also unblocks the codecov check on #443. That PR renames
detecion_blockstodetection_blocksand corrects an "initalized"typo in
EMACallback. Both renamed lines sit in untested code, socodecov counts them as missed and fails
codecov/patch. The new testscover them.
Specification
Use the block widths directly in the PPLCNetV3 detection backbone.
LCNetV3Layerscales itsout_channelsin the constructor. Thebackbone scaled the same value a second time:
The
rec-lightvariant uses the scale 0.95. The second scale turnsthe true block widths
[64, 128, 240, 480]into[64, 128, 224, 464]. The detection convolutions then expect 224channels, but the blocks give 240:
Add
tests/unittests/test_pplcnet_v3.py. It covers the two backbonemodes:
(1, max_text_len).[15, 22, 53, 456].Add
test_ema_before_fit_start. It covers theEMACallback.emaguard. The guard raises before
on_fit_startbuilds the model.No variant sets
use_detection_backbonetotrue. No test coveredthe branch. The defect stayed hidden for these two reasons.
Dependencies & Potential Impact
RuntimeErrorbeforethis change. No configuration can depend on the old widths.
ocr_recognitionpredefined model uses that path.detecion_blocksattribute. Pre-docs cleanup: fix the typo'd names and tighten the lint gate #443 can thus rename the attributewithout a test change.
Deployment Plan
None / not applicable — the changes ship with the next release.
Testing & Validation
test_detection_backbonefails onmainwith theRuntimeErrorabove. It passes with this change.
pytest tests/unittests: 508 passed. The run excludestests/unittests/test_losses, which holds the known expiredgcloud-ADC failures.
pyright --warnings: at parity withmain. The one error is thepre-existing
aimet_torchimport from the optional extra.AI Usage
Assisted-by: Claude:claude-opus-5
Submitted code was reviewed by a human: NO
The author is taking the responsibility for the contribution: YES
🤖 Generated with Claude Code
https://claude.ai/code/session_01QA7RcK3rsE5mHX7J6X1Vxh
Summary by CodeRabbit
Bug Fixes
Tests