thingsvision icon indicating copy to clipboard operation
thingsvision copied to clipboard

Deprecate `module_name` and automatically perform extraction for all available modules?

Open LukasMut opened this issue 2 years ago • 5 comments

LukasMut avatar Oct 09 '22 09:10 LukasMut

Suggestion: Let user provide either None for all modules, str for single module or List for multiple modules. This should also probably come after #74.

andropar avatar Oct 10 '22 09:10 andropar

This sounds reasonable. We should probably only accept None or List[str]. For a single module, the list contains only a single element.

LukasMut avatar Oct 10 '22 09:10 LukasMut

For this, we want to use defaultdict(list) from collections and append features for every module.

LukasMut avatar Oct 27 '22 08:10 LukasMut

I second letting users provide names. Otherwise, some users might be suprised/confused how much of their storage - depending on the network they chose - is now filled with activations for dozens of modules :)

I'm not sure if I'd go with None for all modules though. It might be more intuitive to require the string all. But I can't evaluate whether that makes things ugly or harder to maintain code-wise :) Just my 2 cents.

PhilippKaniuth avatar Oct 27 '22 09:10 PhilippKaniuth

I think making extracting all modules more explicit makes sense, otherwise you could just do it by accident. Maybe the best option would be to only allow List[str] and if the user really wants to extract everything, he could just do module_names=extractor.get_modules_names()? I think that's an edge case anyway

andropar avatar Oct 27 '22 09:10 andropar