-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
refine build_fourier_pos_embed() to use precomputed bands #2650
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: main
Are you sure you want to change the base?
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
@nukyan I've thought about refactoring this a bit, but I don't feel I can/should break compat for the original fn name. So the solution would be to split into two components, have the original fn call those, and refactor the modules to call the separated fn.... In the grid creation I'd also planned to unify the dinov3 style grid (0.5 center w/ normalization) as an option in addition to 'pixels' and the default index based approach. |
Okay, I've already revert back the original function signature. Now those modules uses Additionally, I believe refactoring it is the better choice for the better code quality. But as a newcomer to CV, I'm afraid I can't offer any assistance with this matter. |
|
@nukyan thanks for the update, still thinking about the API a bit and whether it makes sense for me to slot my dinov3 idea on top... Once you have a project with thousands of users and a whole lot of other projects depending on it, you cannot arbitrarily refactor for the sake of improving code quality. You have to weight the likelihood of breaking other people's use of your library against the quality improvement. One cannot assume that people are only using the library functions via the models themselves, many do use sub-modules, functions directly and that's hard to track. With modules, functions like this, when I started I had only a few models using it and as usual, decided to cut off refinement at some point to 'ship it'... and then over time more models started using it and yeah, would have benefited from a better initial design. |
Each positional embedding module has its own method for calculating frequency bands, so this should not be computed within
build_fourier_pos_embed(). And also, for those who wish to implement alternative band calculation methods, simply override the_compute_bands()function. This is a convenient way.