Extend feature extraction module to allow for RASR compatible logmel features #40
Conversation
kuacakuaca
requested review from
albertz,
curufinwe,
christophmluscher,
JackTemaki,
mmz33,
michelwi,
Judyxujj,
SimBe195,
vieting,
ZijYang and
Atticus1806
|
I'm curious, did you actually test this, that you really get identical results to RASR (up to numerical errors) |
there were still small differences. Not sure if that could be further eliminated. |
|
Oh, but that looks very good. Can you maybe link the relevant RASR code for comparison? |
|
…nd_feature_extraction
…ength computation
| periodic: bool = True | ||
| htk: bool = False | ||
| norm: Optional[Union[Literal["slaney"], float]] = "slaney" | ||
| dtype: DTypeLike = np.float32 | ||
| spectrum_type: SpectrumType = SpectrumType.STFT |
There was a problem hiding this comment.
I think we agreed on not setting any defaults unless it is immediately obvious that that should be a default or for "structural parts"
But I now wonder that the original values had no defaults and what you add does. (Wich is required, I understand, to keep backwards compatibility - But then did we not agree to deprecate the old class and create a new one V2 in cases like this?)
Do we actually need to configure all this? Or are there ever only 2 sets of parameters that are going to be used? ("default", "rasr-compatible"). Is then not a better choice to have a single parameter for "default" vs. "rasr-compatible" that then sets all of these never otherwise touched parameters to the appropriate values?
And if the answer is yes, would it not rather make sense in this case to do create a new class - but not LogMelFeatureExtractionV2 but instead RasrCompatibleLogMelFeatureExtractionV1..? (and then we have no config options, just the two classes do use the correct thing)
There was a problem hiding this comment.
curufinwe approved these changes 8 minutes ago
curufinwe merged commit c363a01 into main 8 minutes ago
oh well 🤷
There was a problem hiding this comment.
I don't think the defaults are problematic here.
There was a problem hiding this comment.
It is not the defaults that are problematic, but adding new parameter automatically breaks setups. I will revert the PR. And then I would be in favor of Willis RasrCompatibleLogMelFeatureExtraction instead of the V2. But I would be fine with both.
There was a problem hiding this comment.
Why does adding parameters with default values break anything?
Edit See discussion in #41 about that.
No description provided.