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

adding the other versions of dreamsim #156

Merged
merged 8 commits into from
Mar 18, 2024
Merged

adding the other versions of dreamsim #156

merged 8 commits into from
Mar 18, 2024

Conversation

LukasMut
Copy link
Collaborator

@LukasMut LukasMut commented Mar 11, 2024

This PR adds two new DreamSim models to thingsvision: dino_vitb16 and a DreamSim ensemble.

@LukasMut LukasMut added the enhancement New feature or request label Mar 11, 2024
@Alxmrphi
Copy link
Collaborator

Happy to review but currently out of the office today (back tomorrow) so if it's okay to wait, I will gladly take a look then. No worries if you need to re-assign to anyone else.

@Alxmrphi
Copy link
Collaborator

  1. Unsure why some of the unit tests are failing. Based on the code changes for this PR, it's looks like this is caused by something already in the code base that's not as easy to spot from the POV of someone quickly reviewing this specific PR. So, I won't make reference to that below.

  2. The README and website AvailableModels page needs to be updated to reflect the addition of the models.

General thoughts (non-specific to this PR)

Beyond that, it's a bit confusing to require the model_variants in this way because DreamSim then has quite a bit of different behaviour that doesn't match the given example on the website (in terms of defining the extractor). When extracting activations from 'open_clip_vitb32' then it says I should be able to use model and model.mlp but only the latter one works.

extractor.show_model()

DreamSimModel(
  (model): PerceptualModel(
    (mlp): Identity()
  )
)

KeyError: 'model'

If you specify anything different, you're informed:

ValueError: 
X not a valid module name. Please choose a name from the following set of modules: ['model', 'model.mlp']

... which isn't actually the case (as the tests/helper.py file specifies, namely that only model.mlp is a possibility here). So, there's a bit of potential confusion there for end users which might need a bit of consideration in the future.

Summary 👍

In any case, I was able to get activations from the two additional models (dino_vitb16 and ensemble) without issue from the model.mlp layer so it's only the update of the README and website that need to be updated and all looks good. Other points are general thoughts perhaps for ongoing consideration if more idiosyncratic behaviour is required for additional models in the library.

@LukasMut
Copy link
Collaborator Author

LukasMut commented Mar 18, 2024

  1. Unsure why some of the unit tests are failing. Based on the code changes for this PR, it's looks like this is caused by something already in the code base that's not as easy to spot from the POV of someone quickly reviewing this specific PR. So, I won't make reference to that below.
  2. The README and website AvailableModels page needs to be updated to reflect the addition of the models.

General thoughts (non-specific to this PR)

Beyond that, it's a bit confusing to require the model_variants in this way because DreamSim then has quite a bit of different behaviour that doesn't match the given example on the website (in terms of defining the extractor). When extracting activations from 'open_clip_vitb32' then it says I should be able to use model and model.mlp but only the latter one works.

extractor.show_model()

DreamSimModel(
  (model): PerceptualModel(
    (mlp): Identity()
  )
)

KeyError: 'model'

If you specify anything different, you're informed:

ValueError: 
X not a valid module name. Please choose a name from the following set of modules: ['model', 'model.mlp']

... which isn't actually the case (as the tests/helper.py file specifies, namely that only model.mlp is a possibility here). So, there's a bit of potential confusion there for end users which might need a bit of consideration in the future.

Summary 👍

In any case, I was able to get activations from the two additional models (dino_vitb16 and ensemble) without issue from the model.mlp layer so it's only the update of the README and website that need to be updated and all looks good. Other points are general thoughts perhaps for ongoing consideration if more idiosyncratic behaviour is required for additional models in the library.

Thank you for your thorough review @Alxmrphi! I just updated both README.md and the docs according to your suggestion(s).

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 73.24%. Comparing base (107d317) to head (649123e).

Files Patch % Lines
thingsvision/custom_models/dreamsim/dreamsim.py 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #156   +/-   ##
=======================================
  Coverage   73.24%   73.24%           
=======================================
  Files          30       30           
  Lines        1555     1555           
  Branches      222      222           
=======================================
  Hits         1139     1139           
  Misses        349      349           
  Partials       67       67           
Flag Coverage Δ
unittests 73.24% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LukasMut LukasMut added cleanup Deprecate or refactor code documentation Improvements or additions to documentation and removed cleanup Deprecate or refactor code labels Mar 18, 2024
@LukasMut LukasMut merged commit c88e71c into master Mar 18, 2024
5 of 6 checks passed
@LukasMut LukasMut deleted the integrate-dreamsim branch March 18, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants