Skip to content

Added option to specify list of module names instead of single module name#186

Merged
LukasMut merged 22 commits into
ViCCo-Group:masterfrom
lucaeyring:master
Aug 6, 2025
Merged

Added option to specify list of module names instead of single module name#186
LukasMut merged 22 commits into
ViCCo-Group:masterfrom
lucaeyring:master

Conversation

@lucaeyring

Copy link
Copy Markdown
Collaborator

This PR adds the functionality to save multiple intermediate features in a single forward pass.

This is implemented through a new argument module_names. Now either module_name or module_names need to be supplied. This is done for backwards compatibility (everything should still work with just module_name).

One change needed to be made, which is that features is now a Dict from module_name to corresponding features_module, and the filename, which the features are saved under, now includes the module_name. Need to double check whether this breaks anything w.r.t. backward compatibility.

@lciernik lciernik requested review from LukasMut and lciernik July 29, 2025 12:10
@LukasMut LukasMut requested a review from MarcoMorik July 31, 2025 11:52
@LukasMut LukasMut added the enhancement New feature or request label Jul 31, 2025
@LukasMut LukasMut added the cleanup Deprecate or refactor code label Jul 31, 2025

@lciernik lciernik left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments on where things could be done slightly differently.

Some questions remain:

  • Where is the backward compatibility logic when module_name is given? It appears to me that _extract_batch will, in any case, return a dictionary of module names and features.
  • We can apply alignment of representations. See align method in torch.py. Does this still work with multiple module_names?
  • What happens if the dataset is huge?

Comment thread thingsvision/core/extraction/base.py
Comment thread thingsvision/core/extraction/base.py Outdated
Comment thread thingsvision/core/extraction/base.py Outdated
Comment thread thingsvision/core/extraction/base.py
Comment thread thingsvision/core/extraction/base.py
Comment thread thingsvision/core/extraction/base.py
Comment thread thingsvision/core/extraction/base.py
Comment thread thingsvision/core/extraction/torch.py
Comment thread thingsvision/core/extraction/torch.py Outdated
Comment thread thingsvision/core/extraction/torch.py Outdated

@lciernik lciernik left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still have some comments.

Comment thread thingsvision/core/extraction/base.py
Comment thread thingsvision/core/extraction/base.py
Comment thread thingsvision/core/extraction/base.py
Comment thread thingsvision/core/extraction/torch.py
Comment thread thingsvision/core/extraction/base.py
Comment thread thingsvision/core/extraction/base.py
Comment thread thingsvision/core/extraction/base.py
Comment thread thingsvision/core/extraction/torch.py Outdated
@LukasMut LukasMut added this pull request to the merge queue Aug 6, 2025
Merged via the queue into ViCCo-Group:master with commit 027f3c0 Aug 6, 2025
1 of 3 checks passed
@LukasMut

LukasMut commented Aug 6, 2025

Copy link
Copy Markdown
Collaborator

This fixes issue #89

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup Deprecate or refactor code enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants