remove unused imports and functions + replace print with logger calls #158
Conversation
| if square_resize_div_64: | ||
| logger.info(f"Building Roboflow {image_set} dataset with square resize at resolution {resolution}") | ||
| dataset = CocoDetection(img_folder, ann_file, transforms=make_coco_transforms_square_div_64(image_set, resolution, multi_scale=args.multi_scale, expanded_scales=args.expanded_scales)) | ||
| else: | ||
| logger.info(f"Building Roboflow {image_set} dataset at resolution {resolution}") | ||
| dataset = CocoDetection(img_folder, ann_file, transforms=make_coco_transforms(image_set, resolution, multi_scale=args.multi_scale, expanded_scales=args.expanded_scales)) |
There was a problem hiding this comment.
there is a bug in the existing implementation that is stopping expanded_scales from getting propagated to the dataset when training on roboflow data. you can see it clearly here. this change fixes that bug, which will likely increase performance. however, it'll also slow down training a bit and increase memory usage a bit. not sure that's what we want. worth discussing
There was a problem hiding this comment.
@isaacrob-roboflow what's your suggestion here?
There was a problem hiding this comment.
Want to keep the bug for now but comment that it exists, and then run a proper ablation so we know if it's worth including?
There was a problem hiding this comment.
@isaacrob-roboflow That would be great! I’d really like to get this PR merged soon, since it touches a lot of areas and keeps running into conflicts. Plus, I’d rather not merge it right before the release — it increases the risk that something might go wrong.
Would you like me to add a TODO comment on line 239?
TODO: There is a bug in the current implementation that prevents `expanded_scales` from being propagated to the dataset when training on Roboflow data.
Alternatively, we could create a GitHub issue to track the problem. To be honest, I’d prefer the second option.
There was a problem hiding this comment.
let's do both and tag the issue in the comment?
| # Check if batch size is divisible by gradient accumulation steps | ||
| if batch_size % args.grad_accum_steps != 0: | ||
| logger.error(f"Batch size ({batch_size}) must be divisible by gradient accumulation steps ({args.grad_accum_steps})") | ||
| raise ValueError(f"Batch size ({batch_size}) must be divisible by gradient accumulation steps ({args.grad_accum_steps})") |
There was a problem hiding this comment.
we should be able to handle this case but I am not sure it is being handled correctly right now. nothing fundamental stopping us
There was a problem hiding this comment.
@isaacrob-roboflow My PR didn’t change the behavior here — it just made it consistent with how the logger is used elsewhere. I’m happy to explore better approaches, but that’s outside the scope of this PR.
| # ------------------------------------------------------------------------ | ||
| # RF-DETR | ||
| # Copyright (c) 2025 Roboflow. All Rights Reserved. | ||
| # Licensed under the Apache License, Version 2.0 [see LICENSE for details] | ||
| # ------------------------------------------------------------------------ |
There was a problem hiding this comment.
this file was probably also copied over from LW-DETR .. even though they don't have a trademark in theirs, should we add it for them? (assuming we did copy it .. )
There was a problem hiding this comment.
@isaacrob-roboflow This wasn’t copied from LW-DETR — I created it myself. All the code here is either written by us or by open-source contributors.
# Conflicts: # rfdetr/deploy/export.py
Description
This pull request includes a series of changes primarily focused on replacing print statements with logger calls.