Conversation
src/metrax/metrics.py
Outdated
| total_edit_distance = 0 | ||
| total_reference_length = 0 | ||
|
|
||
| for pred, ref in zip(predictions, references): |
There was a problem hiding this comment.
should we add a TODO to use a more involved tokenizer in the future?
There was a problem hiding this comment.
That's a great point! To keep things in line with the single responsibility principle, I think we should remove the tokenization logic entirely and just accept pre-tokenized lists of strings as input, leaving the tokenizer implementation to the one who calls the metric.
There was a problem hiding this comment.
The metric now accepts only lists of strings. Do you think we should accept strings and implement a tokenizer?
There was a problem hiding this comment.
Hi Nikola! I agree that single responsibility principle is important. However, considering that it is more common for these types of metrics to take in a whole sentence, (for instance, this torchMetrics WER) let's keep the implementation such that we can take in sentences as input.
|
Thank you Nikola for your PR! Left some comments. Feel free to sync the workspace as well! :) |
|
Thank you Jiwon for reviewing my PR! |
|
Thank you Nikola! LGTM given all the comments are resolved. |
|
Hi nikola, a bunch of PRs were submitted to split the metrics.py file into multiple modules. I think the WER class can be placed under the new |
nikolasavic3
force-pushed
the
feat-wer
branch
3 times, most recently
from
a0690cd to
64ea4fe
Compare
src/metrax/nlp_metrics.py
Outdated
| def total_reference_length(self): | ||
| return self.count | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
no need to override this function as it already exists in Average
src/metrax/nlp_metrics.py
Outdated
|
|
||
| return distance_matrix[m][n] | ||
|
|
||
| def merge(self, other: 'WER') -> 'WER': |
There was a problem hiding this comment.
no need to override this function anymore as it exists in Average
src/metrax/nlp_metrics.py
Outdated
| count=self.count + other.count, | ||
| ) | ||
|
|
||
| def compute(self) -> jax.Array: |
There was a problem hiding this comment.
no need to override this function as it exists in Average
src/metrax/nlp_metrics.py
Outdated
| total_edit_distance: Sum of edit distances across all samples. | ||
| total_reference_length: Sum of reference lengths across all samples. | ||
| """ | ||
| @property |
There was a problem hiding this comment.
these can be removed as well since
|
Thank you so much nikola! Almost there :) I left some comments so that we can 1) remove functions that already exist as part of Thank you again Nikola! |
nikolasavic3
force-pushed
the
feat-wer
branch
4 times, most recently
from
a767587 to
24d287d
Compare
|
Thank you for the detailed feedback, Jiwon! I've removed all the redundant function overrides (merge, compute, total_reference_length) and updated the implementation to accept untokenized strings. |
|
Thank you so much Nikola! We really appreciate your contribution to the Metrax codebase! :) |
|
Thank you so much, Jiwon! I really appreciate your detailed feedback and guidance throughout this process. Collaborating with you on adding the WER metric to the Metrax codebase was great. Looking forward to contributing more in the future! |
2 participants
Add Word Error Rate (WER) metric
Fixes #28