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
Make parameter optional string #641
Conversation
Codecov Report
@@ Coverage Diff @@
## master #641 +/- ##
==========================================
- Coverage 88.84% 88.80% -0.04%
==========================================
Files 21 21
Lines 2223 2225 +2
==========================================
+ Hits 1975 1976 +1
- Misses 248 249 +1
Continue to review full report at Codecov.
|
@@ -376,7 +376,7 @@ def create_fb_matrix( | |||
up_slopes = slopes[:, 2:] / f_diff[1:] # (n_freqs, n_mels) | |||
fb = torch.max(zero, torch.min(down_slopes, up_slopes)) | |||
|
|||
if norm == "slaney": | |||
if norm is not None and norm == "slaney": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not allow values other than "slaney"
or None
to prevent the case where unintended value is passed, like typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly looks good.
@vincentqb Test needs to be fixed. |
test/torchscript_consistency_impl.py
Outdated
@@ -83,7 +83,7 @@ def func(_): | |||
f_max = 20.0 | |||
n_mels = 10 | |||
sample_rate = 16000 | |||
norm = "" | |||
norm = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than changing it to None
, I suggest following the original suggestion of passing string value.
Following up on comment