Skip to content

Fix Issue #1277 timit recipe missing uppercase option #1564

Merged
Adel-Moumen merged 4 commits into
speechbrain:developfrom
Adel-Moumen:1277-timit-recipe-missing-uppercase-option-1
Sep 16, 2022
Merged

Fix Issue #1277 timit recipe missing uppercase option #1564
Adel-Moumen merged 4 commits into
speechbrain:developfrom
Adel-Moumen:1277-timit-recipe-missing-uppercase-option-1

Conversation

@Adel-Moumen

@Adel-Moumen Adel-Moumen commented Sep 7, 2022

Copy link
Copy Markdown
Collaborator

Small fix for #1277

@Adel-Moumen Adel-Moumen changed the title 1277 timit recipe missing uppercase option 1 Fix Issue #1277 timit recipe missing uppercase option Sep 7, 2022
@anautsch

Copy link
Copy Markdown
Collaborator

lgtm - some thoughts on an adjacent issue.

This was not detected by yaml tests, although they consider data preparation scripts - why?

check_yaml_vs_script(row["Hparam_file"], row["Script_file"])

While the recipe overview has the data preparation script referenced, it plays a litter role for the yaml testin.
https://github.com/speechbrain/speechbrain/blob/develop/tests/recipes.csv

def check_yaml_vs_script(hparam_file, script_file):

Should testing include that through YAML one could specify parameters of any subfunction?
(e.g., retraversal of uses; namespaces, and if all function arguments are settable then; their default is changeable through YAML.)

@Adel-Moumen

Copy link
Copy Markdown
Collaborator Author

This was not detected by yaml tests, although they consider data preparation scripts - why?

Hmm according to the doc string of

def check_yaml_vs_script(hparam_file, script_file):
this is "normal" that the tests have failed to catch this problem because the tests are only checking if a variable is unused in the python files while in our case we just forgot to declare it and use it. (which is a different problem)

Not sure to understand clearly your last paragraph.

@anautsch

Copy link
Copy Markdown
Collaborator

this is "normal" that the tests have failed to catch this problem because the tests are only checking if a variable is unused in the python files while in our case we just forgot to declare it and use it. (which is a different problem)

You got it; don't worry. I'd see that as a part of functionality for that testing script, whereas it is currently not. It's literally up to us to make it function/or not. Yet the existence of this PR demonstrates: it should be part of function. Question is: what's the least effort here - writing a new testing script; reusing the old one; some hybrid approach - or simply fixing this instance and waiting until it pops up elsewhere again 😉

@Adel-Moumen Adel-Moumen merged commit a139a9a into speechbrain:develop Sep 16, 2022
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

Successfully merging this pull request may close these issues.

2 participants