Skip to content

Match YOLOX and DAMO-YOLO preprocessing to upstream - #24

Merged
parkjinman98 merged 4 commits into
mainfrom
jm/yolox-damo-upstream-letterbox
Oct 1, 2026
Merged

parkjinman98 merged 4 commits into
mainfrom
jm/yolox-damo-upstream-letterbox

Conversation

@parkjinman98

Copy link
Copy Markdown
Contributor

Summary

Upstream's own test transforms are now the source of truth for YOLOX and DAMO-YOLO pre_cfg, replacing mblt-model-ops' pipeline.yaml (which no code there consumes yet).

Step YOLOX upstream (data_augment.preproc) DAMO-YOLO upstream (Dec 2022, 55ae14f) Before this PR
Decode cv2, BGR PIL, RGB cv2 for both
Resized size int(w*r) (truncate) int(w*r) (truncate) round (half-even)
Placement / fill top-left, 114 top-left, 0 same ✅
Box restore / r per axis (BoxList.resize) / r for both

Changes

  • LetterBoxLayout(center, size_rounding, per_axis_ratio) plus letterbox_layout(pre_cfg). These are threaded through every path that derives geometry from shapes alone:

    • PostBase.ratio_pads_for
    • Results plotting
    • dense crops
    • semantic targets
    • DOTA ground truth

    Passing a plain center boolean still works.

  • The LetterBox op accepts size_rounding: floor, per_axis_ratio: true and PIL images. _spatial_shape now reads a PIL image's size, so img0_shape stays the original shape.

  • scale_boxes and scale_coords divide x and y by their own ratio. Uniform layouts are unchanged, since both ratios are equal.

  • Options a task cannot restore now fail loudly:

    • rotated boxes reject a non-uniform ratio;
    • instance segmentation rejects floor and per_axis_ratio;
    • OBB rejects per_axis_ratio.
  • YAML changes:

    • YOLOX: size_rounding: floor.
    • DAMO-YOLO: Reader.style: pil, size_rounding: floor, per_axis_ratio: true.
  • AGENTS.md, the mblt-vision skill and mblt_vision/README.md are updated in the same commit.

  • __version__ is bumped to 0.0.6 so mblt-model-ops' parity pin can move to it.

Test plan

  • New parity tests compare build_preprocess(<model YAML>) pixel-for-pixel with inlined copies of the upstream code. They cover YOLOX-s, YOLOX-Nano and DAMO-YOLO-T, on shapes where truncation and rounding differ. DAMO-YOLO goes through a real JPEG decoded by PIL.
  • Box restoration is checked against upstream: / r for YOLOX, BoxList.resize per axis for DAMO-YOLO.
  • pytest: 735 passed. 5 failed in tests/test_onnx_yolo.py, which download from the Hub; they fail identically on main.
  • pre-commit (ruff 0.6.9) and git diff --check.
  • Not re-measured: the README's COCO scores (YOLOX-s 40.679, DAMO-YOLO-T 41.789) were taken with the old rounding and cv2 decode. No local ONNX or COCO was available, so a re-run is needed. Most COCO val images have a 640 long side (r = 1), so the expected change is small.

🤖 Generated with Claude Code

Upstream's own test transforms are now the source of truth for both families:
YOLOX's data_augment.preproc and DAMO-YOLO's December 2022 Resize +
to_image_list (tinyvision/DAMO-YOLO 55ae14f). Both truncate the resized size
with int(w * r), where we rounded; DAMO-YOLO also decodes with PIL and
restores boxes per axis (BoxList.resize) rather than by one ratio.

- Add LetterBoxLayout (center, size_rounding, per_axis_ratio), read by
  letterbox_layout(pre_cfg) and threaded through every shape-only geometry
  path: PostBase.ratio_pads_for, Results plotting, dense crops, semantic
  targets and DOTA ground truth. The center-boolean form still works.
- LetterBox accepts size_rounding / per_axis_ratio and PIL images;
  _spatial_shape reads a PIL image's size so img0_shape stays the original.
- scale_boxes / scale_coords divide x and y by their own ratio; rotated boxes,
  instance segmentation and OBB reject options they cannot restore.
- YOLOX YAMLs: size_rounding: floor. DAMO-YOLO YAMLs: Reader.style: pil,
  size_rounding: floor, per_axis_ratio: true.
- Tests compare both pipelines pixel-for-pixel with inlined upstream code.
- Bump __version__ to 0.0.6 for the mblt-model-ops parity pin.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@parkjinman98 parkjinman98 self-assigned this Oct 1, 2026
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Codex review

Verdict

⚠️ Two actionable issues remain: DAMO-YOLO COCO evaluation bypasses the PIL decoder, and an exported center= keyword was removed. Fix before merge; focused tests could not run because pytest is unavailable.

Suggested next steps

  • Fix the DAMO-YOLO evaluation decode path and add a JPEG parity test through get_coco_loader.
  • Restore center= compatibility, then run the focused/full pytest suite and remeasure or label the documented COCO scores as historical.

Trigger: pull_request

Comment thread mblt_vision/models/DAMO-YOLO-T.yaml
Comment thread mblt_vision/utils/preprocess/letterbox.py Outdated
- CustomCOCODataset takes decoder="cv2" | "pil", and eval_coco picks PIL for
  a Reader.style: pil model. The loader decodes before preprocessing, so
  DAMO-YOLO evaluation was still fed OpenCV's pixels.
- letterbox_semantic_mask, get_ade20k_loader, get_cityscapes_loader,
  LetterBoxGeometry.from_shapes and resolve_ratio_pad accept the old center=
  keyword again, with a DeprecationWarning; passing both raises TypeError.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@parkjinman98

Copy link
Copy Markdown
Contributor Author

@mobilint-review

Re-review after 28d9549, which fixes both earlier findings (COCO evaluation now decodes with the model's Reader library; the deprecated center= keyword is accepted again on every renamed signature).

Context and intentional decisions, so the review need not re-litigate them:

  • Source of truth for YOLOX/DAMO-YOLO pre_cfg is upstream's own test transform: YOLOX data_augment.preproc; DAMO-YOLO's December 2022 release (tinyvision/DAMO-YOLO 55ae14f), the only checkpoints still downloadable. mblt-model-ops' pipeline.yaml is no longer the reference for these pre_cfgs; its code does not consume them yet.
  • size_rounding: floor (upstream int(w * r)) is a separate option from center, because placement and rounding are independent. Ultralytics models keep round by default and are unchanged.
  • per_axis_ratio applies only to DAMO-YOLO (BoxList.resize); YOLOX restores with / r as upstream does. scale_boxes/scale_coords now divide x and y by their own ratio; for every uniform layout both are equal, so results are identical.
  • Instance segmentation rejects floor/per_axis_ratio and OBB rejects per_axis_ratio on purpose: their restorations cannot honour them, and failing loudly is the repository rule.
  • The README COCO scores predate this change and could not be re-measured here (no local ONNX/COCO); that is noted in the PR description as an open item, not something to fix in code.
  • The 5 failing tests/test_onnx_yolo.py cases need Hub downloads and fail identically on main.
  • __version__ 0.0.6 is the intended bump for mblt-model-ops' parity pin.
    Please focus on correctness of the geometry, the decoder selection, the deprecation shim and the tests.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Codex review

Requested by @parkjinman98.

Verdict

The geometry and COCO decoder changes look correct, but the deprecated center= compatibility shim is still incomplete for renamed geometry helpers.

Suggested next steps

  • Add the deprecated center= alias to the remaining renamed helpers.
  • Add regression tests that call every renamed helper with center=False.

Trigger: issue_comment
Source: #24 (comment)

Comment thread mblt_vision/utils/postprocess/_letterbox.py Outdated
resolve_ratio_pads, _apply_letterbox and DOTA's _ratio_pad_for_shape /
_ground_truth_to_input_space route center= through deprecated_center_argument
like the public helpers; one parametrized test calls every renamed callable
with center=False, layout=..., and both.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@parkjinman98

Copy link
Copy Markdown
Contributor Author

@mobilint-review

Re-review after 4020e81, which completes the deprecated center= shim:

  • Every callable whose center parameter became layout accepts center= again, with a DeprecationWarning: LetterBoxGeometry.from_shapes, resolve_ratio_pad, resolve_ratio_pads, letterbox_semantic_mask, _apply_letterbox, get_ade20k_loader, get_cityscapes_loader, and DOTA's _ratio_pad_for_shape and _ground_truth_to_input_space. Passing both layout and center raises TypeError.
  • One parametrized regression test covers all of them.
  • scale_masks(center=) and nmsout2eval_seg(center=) were never renamed, and keep center.

The guide from the previous request still applies:

  • Upstream's own test transforms are the source of truth.
  • size_rounding and center are separate options on purpose.
  • Instance segmentation and OBB deliberately reject options they cannot restore.
  • The README scores still need re-measuring; that is an open item in the PR description.
  • The 5 test_onnx_yolo.py failures need Hub downloads and fail on main too.
  • The 0.0.6 version bump is intentional.

Please focus on any remaining correctness or test issues.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Codex review

Requested by @parkjinman98.

Verdict

One remaining compatibility defect exists in the deprecated center= shim.

Suggested next steps

  • Use a sentinel to distinguish an omitted layout from explicit layout=True.
  • Extend the parametrized compatibility test to cover layout=True together with center.

Trigger: issue_comment
Source: #24 (comment)

Comment thread mblt_vision/utils/letterbox.py Outdated
The nine shimmed signatures default layout to None, so layout=True together
with center= raises TypeError like any other layout instead of silently
taking center. Omitted still means Ultralytics' default.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@parkjinman98

Copy link
Copy Markdown
Contributor Author

@mobilint-review

Re-review after 54f00b5, which fixes the shim's handling of an explicit layout=True:

  • layout now defaults to None (omitted) in every shimmed signature.
  • An explicit layout, including True, together with center= raises TypeError.
  • Omitted still means Ultralytics' default.
  • Tests cover True, False and a LetterBoxLayout combined with center, for all nine callables.

crop_letterbox(layout=True) is new in this PR and was never spelled center, so it deliberately has no shim.

The earlier guide still applies:

  • Upstream's own test transforms are the source of truth.
  • size_rounding and center are separate options on purpose.
  • Instance segmentation and OBB deliberately reject options they cannot restore.
  • The README scores still need re-measuring; that is an open item.
  • The 5 test_onnx_yolo.py failures need Hub downloads and fail on main too.
  • The 0.0.6 version bump is intentional.

Please focus on any remaining correctness or test issues.

@parkjinman98
parkjinman98 merged commit ab2e80c into main Oct 1, 2026
6 checks passed
@parkjinman98
parkjinman98 deleted the jm/yolox-damo-upstream-letterbox branch October 1, 2026 06:31
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