Skip to content
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

Update table 1 in README to contain SigLIP, DFN #729

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mitchellnw
Copy link
Contributor

No description provided.

@rwightman
Copy link
Collaborator

@mitchellnw wouldn't it be better to highlight the best of each and use the 336, 384, and 378 res results for openai, siglip, dfn ?

@gabrielilharco
Copy link
Collaborator

So I think the current table nicely highlights the models trained on OpenCLIP. I worry about it becoming too polluted if we keep adding new rows when models come out.

That said, I think it's awesome that OpenCLIP is making it so easy to access tons of models trained by others. One suggestion is to keep the table as is, and add the IN/avg acc scatter plot (and maybe the retrieval one also) to the readme. That way we can highlight that we support so many models, how good the best ones are, and point to the big table if readers need more details.

@ludwigschmidt
Copy link
Collaborator

@gabrielilharco I think that some people come to OpenCLIP not to train models, but to easily use current SotA models. For them, it's less relevant what models were trained with OpenCLIP, and more important to quickly see the best models available in the repository. While the scatter plot is a really great overview of the models we have in OpenCLIP, it doesn't quickly tell people what the best models are (the plot is a bit complex). I think having a few rows in the table with the best models available (maybe even with the specific pretrained model names to load them via OpenCLIP) is a good addition. We've received this feedback in the past, so let's take it into account (e.g., https://twitter.com/giffmana/status/1707664238557221285?s=20).

@ludwigschmidt
Copy link
Collaborator

Having said that, I'm also a big supporter of adding the comprehensive scatter plot as long as we update the table with the best models :-) (probably with the higher resolutions as @rwightman suggested).

@gabrielilharco
Copy link
Collaborator

I agree that it's good to make clear what the best models supported by this codebase is. I think
the comment from Lucas reinforces what I'm trying to say though, we should be careful not to add too much information here because it can make it harder for people to find the information they need.

Knowing which models are best is easy from looking at the big tables (which are sorted by perf). I think we can highlight that better on the readme with some writing changes and the plots

@ludwigschmidt
Copy link
Collaborator

In the abstract, I agree with the point about too many models causing confusion. Concretely here, the updated table would have 12 models, which seems manageable and is in line with Luca's suggestion of around 10 key models. Note that the proposal here is not to keep expanding the list but to keep it to around 10 key models (and the best models available in the repo should count as key models). So I don't think we need to worry right now about potential future expansions making the table bigger.

Knowing which models are best is easy from looking at the big tables (which are sorted by perf). I think we can highlight that better on the readme with some writing changes and the plots

I think if a reader needs to go to from the README to another table or parse a complex plot, finding the best model available is more complicated than it needs to be. But I may also be misunderstanding your proposal! If you make another PR we can compare :-)

@gabrielilharco
Copy link
Collaborator

As a compromise, how about we add a single new model (the best one, DFN H/14@376) and remove an old one (maybe L/14 trained on LAION)?

@ludwigschmidt
Copy link
Collaborator

Also good with me! I'm also still OK with 12 rows if others are OK with that.

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.

None yet

4 participants