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

_feature as log attribute in LibrosaFeatureExtractor? #427

Open
rbroc opened this issue Oct 28, 2020 · 3 comments
Open

_feature as log attribute in LibrosaFeatureExtractor? #427

rbroc opened this issue Oct 28, 2020 · 3 comments

Comments

@rbroc
Copy link
Collaborator

rbroc commented Oct 28, 2020

Currently, the feature argument for LibrosaFeatureExtractor (which selects the target librosa feature) is not stored in log_attributes.
It may make sense to do so, as it is otherwise not possible to initialize multiple extractors for different librosa features if cache_transformers is set to True.

@tyarkoni
Copy link
Collaborator

I think the original idea was that users would initialize the subclasses rather than the base class, in which case it wouldn't matter (you'd always have the class name to go by). But since we don't actually prevent instantiation of the base class (and I see no reason to do that), I agree that this is problematic.

If we want to add _feature to logging, probably best to rename that to feature_name throughout, for clarity. Feel free to open a PR.

@tyarkoni
Copy link
Collaborator

tyarkoni commented Oct 28, 2020

Re: the general question of whether to have a single class or multiple classes for wrapping librosa, IIRC we went back and forth on that, and ultimately decided with the current explicit approach. But we could always revisit that decision if, e.g., librosa keeps adding new features, and we don't want to have to keep maintaining a whole hierarchy of wrappers.

[EDIT: on further reflection, I think the main reason for going with the present approach was that some extractors need a bit of custom code to deal with the outputs, which is still the case.]

@rbroc
Copy link
Collaborator Author

rbroc commented Oct 29, 2020

okay let's maybe leave this open for now, not super high-priority and we can maybe come back to it later on.
For reference, the usage case here was the attempt to pass different librosa extractors to a graph in a list created via list comprehension (e.g. [LibrosaFeatureExtractor(feature=f) for f in feature_list]). Being able to do so is convenient in this case, but not so crucial

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