Making TDT models support all-positive durations (previously duration must contain 0) #9656
Conversation
…ions for TDT Signed-off-by: Hainan Xu <hainanx@nvidia.com>
…ble_tdt_durations
Signed-off-by: hainan-xv <hainan-xv@users.noreply.github.com>
Signed-off-by: Hainan Xu <hainanx@nvidia.com>
Signed-off-by: Hainan Xu <hainanx@nvidia.com>
| @pytest.mark.unit | ||
| @pytest.mark.parametrize('device', DEVICES) | ||
| def test_case_randomized_act_label_no_0_duration(self, device): | ||
| if device == 'cuda': |
There was a problem hiding this comment.
I would suggest explicitly skipping the test for "cpu" here and in other tests for TDT.
I see that TDT is not implemented for CPU, but pytest logs show TestTDTLoss::test_case_randomized_act_label_no_0_duration[cpu] PASSED which is confusing.
| - sigma | ||
| + logp_duration(duration_acts, maxT, maxU, num_durations, b, T - 1, U - 1, 1) | ||
| ) | ||
| if durations[0] == 1: |
There was a problem hiding this comment.
Just a suggestion (at your discretion):
duration_1_idx = ... # find index or -1
if duration_1_idx != -1:
betas[offset + (T - 1) * maxU + U - 1] = ... # set betasMotivation: avoid duplicating complex expression.
|
@hainan-xv Something is wrong with tests in this branch (not related to TDT). Please, update to main and toggle |
| @@ -961,7 +961,9 @@ def compute_tdt_alphas_kernel( | |||
| if t > 0 and t < T: | |||
There was a problem hiding this comment.
Git not letting me comment but remove the comment from 945. I don't know why we left that lying around if it's not in use.
There was a problem hiding this comment.
Also, let's just remove num_durations as a variable since it's redundant with also specifying which duration lengths.
Please also update the doc args
| ) # t = 0, so it must be duration = 0. Therefore the last argument passed to logp_duration() is 0. | ||
| ) | ||
| else: | ||
| alphas[offset + u] = -INF | ||
|
|
||
| # now we have t != 0 and u != 0, and we need to consider both non-blank and blank emissions. | ||
| elif t > 0 and t < T: |
There was a problem hiding this comment.
Dumb question: what do we do in cases t<0 are those just skipped passes on the kernel call?
| @@ -961,7 +961,9 @@ def compute_tdt_alphas_kernel( | |||
| if t > 0 and t < T: | |||
| alphas[offset + t * maxU + u] = -INF | |||
|
|
|||
| for i in range(1, num_durations): # skip 0 since blank emission has to advance by at least one | |||
| for i in range(0, num_durations): | |||
There was a problem hiding this comment.
just make it single variable range. No need to make it overt
|
|
||
| # then we add the scores for duration > 1, if such durations are possible given the audio lengths. | ||
| for i in range(2, num_durations): | ||
| for i in range(0, num_durations): |
There was a problem hiding this comment.
just use single variable range
| @@ -1140,11 +1159,12 @@ def compute_tdt_betas_kernel( | |||
| # u == U - 1, we only consider blank emissions. | |||
| if t >= 0 and t + 1 < T: | |||
| betas[offset + t * maxU + U - 1] = -INF | |||
| for i in range(1, num_durations): | |||
| for i in range(0, num_durations): | |||
There was a problem hiding this comment.
use single variable range
What does this PR do ?
This PR makes TDT models support all-positive durations (previously duration must contain 0).
Collection: [ASR]
Changelog
Usage
N/A
GitHub Actions CI
The Jenkins CI system has been replaced by GitHub Actions self-hosted runners.
The GitHub Actions CI will run automatically when the "Run CICD" label is added to the PR.
To re-run CI remove and add the label again.
To run CI on an untrusted fork, a NeMo user with write access must first click "Approve and run".
Before your PR is "Ready for review"
Pre checks:
PR Type:
If you haven't finished some of the above items you can still open "Draft" PR.
Who can review?
Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.
Additional Information