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

Please revise japanese clip's ReadMe and the position of model.eval in cli.py #84

Closed
jackee777 opened this issue Feb 24, 2023 · 2 comments

Comments

@jackee777
Copy link

jackee777 commented Feb 24, 2023

I wonder if one intorduction and one code may be something wrong.

At first, japanese clip's ReadMe is not correct.
The exiting example is following.

python3 clip_benchmark/cli.py \
  --model_type "ja_clip" \ # flag to use japanese-clip
  --pretrained "rinna/japanese-cloob-vit-b-16" \ # now, we have `rinna/japanese-cloob-vit-b-16` or `rinna/japanese-clip-vit-b-16`. 
  --language "jp" \
  --task "zeroshot_classification"  \
  --dataset "imagenet1k" 
  --dataset_root {ROOT_PATH} 

However, it lacks eval/build and "\".
Maybe, the following is correct. In addition to this, "#" or unnecessary spaces prevent executing this shell script.
Please check it.

python3 clip_benchmark/cli.py eval \
  --model_type "ja_clip" \
  --pretrained "rinna/japanese-cloob-vit-b-16" \
  --language "jp" \
  --task "zeroshot_classification"  \
  --dataset "imagenet1k" \
  --dataset_root {ROOT_PATH} 

Secondly, model.eval() in line. 181 of cli.py seems like unnecesary.
model.eval() has alerady been executed in load_clip() function.

I hope you to review them.

@mehdidc
Copy link
Collaborator

mehdidc commented Mar 2, 2023

Thanks @jackee777! you are right, will update the example. On the other hand not sure to see where model.eval() is called in load_clip. I checked model.training after load_clip, and it is still True, so mode.eval() would still be necessary (it is needed for models using dropout and/or batchnorm).

@jackee777
Copy link
Author

@mehdidc
Thank you for your response. You are correct, and I'm sorry for misunderstanding the 2nd question.
I may have seen load_clip in another version of this repository. The current version works successfully.

I close this comment.

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

No branches or pull requests

2 participants