-
Notifications
You must be signed in to change notification settings - Fork 757
Integration of dinov3, including optional train and inference scripts. #324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 1 commit
3391b86
5c0dd43
82d5396
e454223
87a3092
80e72bc
2fb7959
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,14 +4,39 @@ | |||||||||||
| # Licensed under the Apache License, Version 2.0 [see LICENSE for details] | ||||||||||||
| # ------------------------------------------------------------------------ | ||||||||||||
|
|
||||||||||||
| from pydantic import BaseModel, field_validator, model_validator, Field | ||||||||||||
| from pydantic_core.core_schema import ValidationInfo # for field_validator(info) | ||||||||||||
| from typing import List, Optional, Literal | ||||||||||||
| import os, torch | ||||||||||||
|
||||||||||||
| import os, torch | |
| import os | |
| import torch |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's inconsistent spacing in the dictionary definition - missing space after comma on line 30 between "dinov2_windowed_small","dinov2_windowed_base". Add a space after the comma for consistency with Python style conventions.
| "dinov2_windowed_small","dinov2_windowed_base", | |
| "dinov3_small","dinov3_base","dinov3_large" | |
| "dinov2_windowed_small", "dinov2_windowed_base", | |
| "dinov3_small", "dinov3_base", "dinov3_large" |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normal methods should have 'self', rather than 'cls', as their first parameter.
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normal methods should have 'self', rather than 'cls', as their first parameter.
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normal methods should have 'self', rather than 'cls', as their first parameter.
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The field_validator for 'dinov3_repo_dir' and 'dinov3_weights_path' at line 89 accesses info.field_name, but this may fail if the field_name attribute doesn't match the expected keys in the env_map dictionary. Consider adding error handling or verification that the field_name is one of the expected values.
| env_key = env_map[info.field_name] | |
| field_name = getattr(info, "field_name", None) | |
| env_key = env_map.get(field_name) | |
| if not env_key: | |
| return v |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normal methods should have 'self', rather than 'cls', as their first parameter.
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normal methods should have 'self', rather than 'cls', as their first parameter.
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validator checks if len(v) is in the tuple (2,), but using a single-element tuple is unusual. Either use len(v) == 2 for clarity, or if multiple lengths are intended to be valid in the future, document why only 2 is acceptable.
| return v if len(v) in (2,) else [8, 11] | |
| return v if len(v) == 2 else [8, 11] |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The model_validator modifies the model's attributes directly (self.patch_size, self.positional_encoding_size, etc.). While Pydantic v2 allows this in 'after' mode validators, be aware that this can make the config initialization behavior less predictable. Consider documenting this auto-correction behavior prominently or providing a way to disable it for users who want explicit control.
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The print statement at line 133 will execute at import time (when the class is defined), not when an instance is created. This will print the default encoder value every time the module is imported, which is unexpected behavior. Consider moving this to init or removing it entirely if it's just for debugging.
| print("Using RFDETRBaseConfig with encoder:", encoder) |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default out_feature_indexes is changed from [2, 5, 8, 11] to [2, 4, 5, 9], but there's a validator at lines 101-108 that forces it to [8, 11] when using dinov3 encoders. This creates inconsistency and makes it unclear what the actual indexes will be. Consider documenting why these specific indexes were chosen and whether the default should be different for v2 vs v3.
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented-out code should be removed. If documentation about the resolution change is needed, add a clear comment explaining why 512 is used instead of 560/504/576.
| #resolution: int = 504 # 560//16=35 when dinov3_* is used | |
| resolution: int = 512 # 512//16=32 → pos grid auto=32 for both v2/v3 | |
| resolution: int = 512 # unified 512px input; 512//16=32 so pos grid=32 for both v2/v3, replacing earlier 504/560/576 variants |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented-out code should be removed. If documentation about the resolution change is needed, add a clear comment explaining why 512 is used instead of 504/576.
| #resolution: int = 504 # 576//16=36 → pos grid auto=36 | |
| # Use 512 instead of earlier 504/576 choices; keeps a 16-pixel patch (512//16=32) | |
| # while retaining a positional grid size of 36 (from 576//16=36) for compatibility. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |||||||||||||||||
| import sys | ||||||||||||||||||
| from typing import Iterable | ||||||||||||||||||
| import random | ||||||||||||||||||
|
|
||||||||||||||||||
| from contextlib import nullcontext | ||||||||||||||||||
S-Mahoney marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||
| import torch | ||||||||||||||||||
| import torch.nn.functional as F | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -39,12 +39,16 @@ | |||||||||||||||||
| from rfdetr.util.misc import NestedTensor | ||||||||||||||||||
| import numpy as np | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def get_autocast_args(args): | ||||||||||||||||||
| """Return autocast arguments based on the DEPRECATED_AMP flag and args.""" | ||||||||||||||||||
| use_cuda = torch.cuda.is_available() | ||||||||||||||||||
| enabled = bool(getattr(args, "amp", False) and use_cuda) | ||||||||||||||||||
| if DEPRECATED_AMP: | ||||||||||||||||||
| return {'enabled': args.amp, 'dtype': torch.bfloat16} | ||||||||||||||||||
| return {"enabled": enabled, "dtype": torch.bfloat16} | ||||||||||||||||||
| else: | ||||||||||||||||||
| return {'device_type': 'cuda', 'enabled': args.amp, 'dtype': torch.bfloat16} | ||||||||||||||||||
|
|
||||||||||||||||||
| # only use CUDA autocast when CUDA exists | ||||||||||||||||||
| return {"device_type": "cuda", "enabled": enabled, "dtype": torch.bfloat16} | ||||||||||||||||||
|
Comment on lines
+49
to
+50
|
||||||||||||||||||
| # only use CUDA autocast when CUDA exists | |
| return {"device_type": "cuda", "enabled": enabled, "dtype": torch.bfloat16} | |
| # use CUDA autocast when CUDA exists, otherwise fall back to CPU | |
| return { | |
| "device_type": "cuda" if use_cuda else "cpu", | |
| "enabled": enabled, | |
| "dtype": torch.bfloat16, | |
| } |
S-Mahoney marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation of the autocast block and its contents appears to be changed. While the torch.inference_mode(False) wrapper was added for safety, the call to torch.set_grad_enabled(True) at line 133 is redundant since torch.inference_mode(False) already ensures gradients are enabled. Consider removing the torch.set_grad_enabled(True) call to simplify the code.
Uh oh!
There was an error while loading. Please reload this page.