Type hinting and general code review #46
Conversation
spafe/features/mfcc.py
Outdated
| ) | ||
|
|
||
|
|
||
| # TODO: the mel conversion approaches are only ["Oshaghnessy", "Lindsay"] |
There was a problem hiding this comment.
Some conversion approaches were not very stable so I restricted the choices to the ones I deemed stable.
There was a problem hiding this comment.
Well then maybe you should remove references to the Beranek approach then? (If I understand everything correctly).
There was a problem hiding this comment.
I would rather not delete things and leave it to the user to decide :) so I will mention the Beranek appraoch too.
spafe/utils/preprocessing.py
Outdated
| @@ -132,6 +141,9 @@ def windowing(frames, frame_len, win_type="hamming"): | |||
| https://superkogito.github.io/blog/2020/03/13/spectral_leakage_and_windowing.html | |||
|
|
|||
| """ | |||
| # WARNING : I would argue that defaulting to hamming is good, | |||
| # but defaulting _silently_ is bad (any spelling mistake made would result | |||
| # in a silent usage of hamming) | |||
There was a problem hiding this comment.
What do you recommend to do here instead? I am a bit confused by your point.
There was a problem hiding this comment.
Two possible behaviors IMHO:
- since there's already a default argument in the function, remove the
getand straight up use a regularwindow_fn[window_type](getitem) syntax. Any badly written window type will raise aKeyErrorand it's a very quick fix for the user. - keep the same behavior and fallback to hamming, but issue a
warnings.warnto notify the user that this window type is being used.
I'd rather go for behavior 1, but it's your call on this one.
There was a problem hiding this comment.
Option 1 seems better so I went with that. Thank you :))
There was a problem hiding this comment.
Great remarks @hadware 👏 I already attempted to include type-hinting too but some scripts made it so hard (converters.py). However, you solved this elegantly in the code. I tried to implement all you remarks and notes and I will go over this again later today in case I missed something :)
|
Sorry, i've been quite busy, i'll review the PR this week end if I get the time to do so |
|
@hadware any update on this :) ? |
|
Alright, sorry for the extreme delay: I merged the commits from your PR at hadware#1 into this branch. I think I'm out of nitpicking regarding the code, so this is a |
Reference Issue
This is part of the code review for JOSS: openjournals/joss-reviews#4739
This PR is based on the edits from #44 . That other PR should probably be merged before this one.
What does this implement/fix? Explain your changes.
This PR has has three aims:
# TODOor# WARNINGcomment for you to solve. You can probably solve them by directly pushing in this PR if you wish