bmcfee / pumpp

practically universal music pre-processor
ISC License
60 stars 11 forks source link

Default names for transformers ; make all parameters keyword-only #114

Closed beasteers closed 2 years ago

beasteers commented 5 years ago

name is the first parameter across all feature extractors, but it seems unnecessary in many situations. I propose that we should make it a default parameter and have it default to self.__class__.__name__ or whatever, like how keras does it (and maybe do camel2snake case). Obviously this would require moving the argument position which would be a breaking change so I understand the hesitation to do anything about it. It's just messing with my global parameter filling a bit lol

bmcfee commented 5 years ago

Can you describe situations in which you wouldn't want a name for each feature extractor?

The point here is to provide a way of (hierarchically) grouping features together where there could otherwise be namespace conflicts. For instance, if you for some reason wanted to have a CQT and STFT together, they could both have mag and phase features, so the only thing keeping them separate is the name scope.

beasteers commented 5 years ago

right, but couldn't we infer the name from the feature extractor?

Like by default, we could name them cqt/mag and stft/mag. I feel like we shouldn't need to explicitly name them for simple cases like tthat. If you wanted two STFT then you could name them explicitly name='stft_1' and name='stft_2'

Or what would be fun would be the automatically incrementing the names like tensorflow does, but that's probably a bit extra.

bmcfee commented 5 years ago

Oh i see -- are you then suggesting that we have default names for the feature extractors? I could be okay with that.

beasteers commented 5 years ago

Yep! Something like this:

from pumpp.feature import STFT, Mel, STFTPhaseDiff
import inflection

for cls in [STFT, Mel, STFTPhaseDiff]:
    print(inflection.underscore(cls.__name__), cls)
stft <class 'pumpp.feature.fft.STFT'>
mel <class 'pumpp.feature.mel.Mel'>
stft_phase_diff <class 'pumpp.feature.fft.STFTPhaseDiff'>
bmcfee commented 5 years ago

Well, I don't think we need to get fancy with this: explicit strings are better IMO. And we're only talking about a handful of classes anyway, so may as well hard-wire it in for the sake of readability.

The only hiccup is around default args and positionals: we can do this if we make all arguments keyword-only, which I generally like stylistically.

beasteers commented 5 years ago

Sure, that's fine by me. I was more just demonstrating the pattern.

And yeah I agree about kw-only. I think in cases like this it totally makes sense (what should the first argument to Mel be anyways?).