FocalCodec [NeurIPS 2025]#3000
Conversation
mravanelli
left a comment
There was a problem hiding this comment.
Great Job @lucadellalib! I did a quick code inspection and shared some comments about the docstrings.
Other Comments:
- Regarding the extra-depenency, please take a look at our policy here. This code should be compliant with that. @pplantinga can advise
- I'm not sure about having "metrics" as a local folder. I think we might need to resuse the same metrics in other recipes, for instance the streamable focalcodec and the extension of FocalCoded to LibriLight. Maybe we can put it in SpeechBrain/metrics?. Any advise @pplantinga ?
|
|
||
|
|
||
| class Generation(sb.Brain): | ||
| def fit_batch(self, batch): |
There was a problem hiding this comment.
For every method, we typically add some short description about its functionality for better clarity (See for instance this). It is even more important here as some methods are not standard.
| return super()._fit_valid(valid_set, epoch, enable) | ||
|
|
||
| @torch.no_grad() | ||
| def evaluate_batch(self, batch, stage): |
There was a problem hiding this comment.
Make sure every method has a short description.
recipes/LibriTTS/focalcodec/utils.py
Outdated
|
|
||
|
|
||
| def prepare_recipe(hparams, run_opts): | ||
| # Dataset preparation |
recipes/LibriTTS/focalcodec/utils.py
Outdated
| audio_backend="soundfile", | ||
| **kwargs, | ||
| ): | ||
| """This function prepares the datasets to be used in the brain class. |
There was a problem hiding this comment.
Improve the docstring by explaining all the parameters. They are a lot in this case, but that can improve clarity and usability.
recipes/LibriTTS/focalcodec/utils.py
Outdated
| provides = ["sig"] | ||
|
|
||
| def audio_pipeline_train(wav): | ||
| original_sample_rate = sb.dataio.dataio.read_audio_info(wav).sample_rate |
There was a problem hiding this comment.
Make sure all the functions have a short docstring
|
|
||
|
|
||
| class DWER(MetricStats): | ||
| def __init__( |
There was a problem hiding this comment.
Add docstring with a working example
|
|
||
|
|
||
| class SpkSimWavLM(MetricStats): | ||
| def __init__( |
There was a problem hiding this comment.
Add a docstring with a working example (such that we can test all with our doc tests)
|
|
||
|
|
||
| class UTMOS(MetricStats): | ||
| def __init__(self, sample_rate, model=None): |
There was a problem hiding this comment.
Add docstring with example
speechbrain/lobes/models/HifiGAN.py
Outdated
|
|
||
|
|
||
| class HingeGLoss(nn.Module): | ||
| """Hinge Generator Loss |
There was a problem hiding this comment.
Add the example to the docstring
speechbrain/lobes/models/HifiGAN.py
Outdated
|
|
||
|
|
||
| class HingeDLoss(nn.Module): | ||
| """Hinge Discriminator Loss |
I think this PR follows the policy: for recipes, the only requirement is an
Perhaps we can leave it here for now, and move it if we do end up using it for other recipes. The principle of YAGNI (you ain't gonna need it) might apply here, let's keep it as straightforward as possible and not plan too far ahead. |
|
Thank you for your comments @pplantinga! Do you have other comments or suggestions? |
|
@Adel-Moumen, do you also have some comments and suggestions here? |
|
I tested the recipe and the recipe tests. All seems to work properly.
|
|
This PR LGTM now. I think we can go ahead and merge it, unless @pplantinga or @Adel-Moumen have further comments. Great Job @lucadellalib! |
Add FocalCodec training recipe.